Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/nw23001440/zadd result array #407

Merged
merged 6 commits into from
Feb 1, 2024

Conversation

davidepalladino-apuliasoft
Copy link
Collaborator

@davidepalladino-apuliasoft davidepalladino-apuliasoft commented Jan 31, 2024

Description

To fix this issue, I improved CsZ_ADDContext.toAst by passing this.cspec_fixed_standard_parts().result.toAst(conf) instead DataRefExpr(ReferenceByName(name), position) for the first parameter (target) of ZAddStmt.

Related to:
T10_A20_P19

Checklist:

  • There are tests regarding this feature
  • The code follows the Kotlin conventions (run ./gradlew ktlintCheck)
  • The code passes all tests (run ./gradlew check)
  • There is a specific documentation in the docs directory

Copy link
Collaborator

@lanarimarco lanarimarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good practices:

  • understanding the parse tree
  • make yourself this question: is there an extension function parseTreeNode.toAst that I can use?

In practice:
Add to your test:
assertCanBeParsed(exampleName = "smeup/T10_A20_P19", printTree = true)
When you run it you will see:

<csZ_ADD>
  Z-ADD
  <cspec_fixed_standard_parts>
    <factor>
      <factorContent>
        10
      </factorContent>
    </factor>
    <resultType>
      A20_AR1(1)
    </resultType>
    <resultIndicator/>
    <resultIndicator/>
    <resultIndicator/>
  </cspec_fixed_standard_parts>
</csZ_ADD>

This is the part of the parse tree for the Z-ADD operation code.
How you can see the result is inside this node:

<resultType>
  A20_AR1(1)
</resultType>

If you remember, for each node in the parse tree, antlr will create a class named <NodeName>Context, in this case: ResultTypeContext.

If you search for ResultTypeContext.toAst you can use it because this method converts a parse tree node (instance of org.antlr.v4.runtime.tree.Tree) into the generic ast node (instance of com.strumenta.kolasu.model.Node).
In this case the concrete class returned by ResultTypeContext.toAst is com.smeup.rpgparser.parsing.ast.AssignableExpression

Try to use these suggestions and you will see that your implementation costs one or two lines of code

@davidepalladino-apuliasoft
Copy link
Collaborator Author

Good practices:

* understanding the parse tree

* make yourself this question: is there an extension function `parseTreeNode.toAst` that I can use?

In practice: Add to your test: assertCanBeParsed(exampleName = "smeup/T10_A20_P19", printTree = true) When you run it you will see:

<csZ_ADD>
  Z-ADD
  <cspec_fixed_standard_parts>
    <factor>
      <factorContent>
        10
      </factorContent>
    </factor>
    <resultType>
      A20_AR1(1)
    </resultType>
    <resultIndicator/>
    <resultIndicator/>
    <resultIndicator/>
  </cspec_fixed_standard_parts>
</csZ_ADD>

This is the part of the parse tree for the Z-ADD operation code. How you can see the result is inside this node:

<resultType>
  A20_AR1(1)
</resultType>

If you remember, for each node in the parse tree, antlr will create a class named <NodeName>Context, in this case: ResultTypeContext.

If you search for ResultTypeContext.toAst you can use it because this method converts a parse tree node (instance of org.antlr.v4.runtime.tree.Tree) into the generic ast node (instance of com.strumenta.kolasu.model.Node). In this case the concrete class returned by ResultTypeContext.toAst is com.smeup.rpgparser.parsing.ast.AssignableExpression

Try to use these suggestions and you will see that your implementation costs one or two lines of code

Thank you Marco for your suggestion. If I understand correctly, I could change line 1414 of misc.kt

return ZAddStmt(resultExpression, dataDefinition, expression, position)

in

return ZAddStmt(this.cspec_fixed_standard_parts().result.toAst(conf), dataDefinition, expression, position)

removing unused code, as I did in last today's commit.

I look your feedback,
Davide.

@lanarimarco
Copy link
Collaborator

lanarimarco commented Feb 1, 2024 via email

@davidepalladino-apuliasoft
Copy link
Collaborator Author

davidepalladino-apuliasoft commented Feb 1, 2024

Thank you. I did it in new commit.

Copy link
Collaborator

@lanarimarco lanarimarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another little step...

Copy link
Collaborator

@lanarimarco lanarimarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that also the PR comment must be revised.

Copy link
Collaborator

@lanarimarco lanarimarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@lanarimarco lanarimarco merged commit c8dfb39 into develop Feb 1, 2024
3 checks passed
@davidepalladino-apuliasoft davidepalladino-apuliasoft deleted the bugfix/NW23001440/zadd_result_array branch February 1, 2024 11:06
@davidepalladino-apuliasoft davidepalladino-apuliasoft removed their assignment Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants