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

refactor(AST): introduce AstVisitor trait #1231

Merged
merged 11 commits into from
Jun 12, 2024
Merged

refactor(AST): introduce AstVisitor trait #1231

merged 11 commits into from
Jun 12, 2024

Conversation

riederm
Copy link
Collaborator

@riederm riederm commented May 31, 2024

The AST-Visitor trait allows generic visiting of AST-nodes. A default Walking behavior is implemented for each AstStatement but it can be altered by any implementor. When overriding a visit_XXX method, the implementation can decide to continue with the default walking behavior (by calling the walk function on the passed AstStatement-Struct, to skip it, or to continue with an alternative walking bahavior.

The code could be improved with a derive-macro for the walker trait

@riederm riederm force-pushed the mari/ast_visitor branch 2 times, most recently from 6d6a228 to ebae171 Compare May 31, 2024 22:02
The AST-Visitor trait allows generic visiting of AST-nodes. A default
Walking behavior is implemented for each AstStatement but it can be
altered by any implementor. When overriding a visit_XXX method, the
implementation can decide to continue with the default walking behavior
(by calling the walk function on the passed AstStatement-Struct, to skip it,
or to continue with an alternative walking bahavior.
Copy link

codecov bot commented Jun 1, 2024

Codecov Report

Attention: Patch coverage is 64.91228% with 80 lines in your changes missing coverage. Please review.

Project coverage is 90.88%. Comparing base (fedc06d) to head (4776e10).
Report is 16 commits behind head on master.

Current head 4776e10 differs from pull request most recent head ac26a8c

Please upload reports for the commit ac26a8c to get more accurate results.

Files Patch % Lines
compiler/plc_ast/src/visitor.rs 64.91% 80 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
- Coverage   95.62%   90.88%   -4.74%     
==========================================
  Files         150      154       +4     
  Lines       42657    45149    +2492     
==========================================
+ Hits        40789    41033     +244     
- Misses       1868     4116    +2248     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@riederm riederm marked this pull request as draft June 1, 2024 12:06
riederm and others added 5 commits June 2, 2024 18:33
the visitor can now be called on the CompilationUnit
and it will visit all GlobalVariables, all Pous, all
UserTypes and Implementations
removed unused AST element CastStatement
@riederm riederm marked this pull request as ready for review June 7, 2024 15:02
@riederm
Copy link
Collaborator Author

riederm commented Jun 7, 2024

I improved the code coverage quite a bit, I think the codecov comment above (64.9%) is outdated.

@riederm riederm requested review from ghaith, volsa and mhasel and removed request for ghaith and volsa June 7, 2024 15:04
src/parser/tests/ast_visitor_tests.rs Show resolved Hide resolved
compiler/plc_ast/src/visitor.rs Outdated Show resolved Hide resolved
compiler/plc_ast/src/visitor.rs Outdated Show resolved Hide resolved
compiler/plc_ast/src/visitor.rs Show resolved Hide resolved
Copy link
Member

@volsa volsa left a comment

Choose a reason for hiding this comment

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

I dig this, especially after seeing how easy and seamless implementing extensions(?) like the IdentifierCollector and FieldCollector appears to be. Really cool to see 🥳

What's the next step? Integrating it into modules like the resolver?

compiler/plc_ast/src/visitor.rs Outdated Show resolved Hide resolved
compiler/plc_ast/src/visitor.rs Show resolved Hide resolved
Copy link
Member

@mhasel mhasel left a comment

Choose a reason for hiding this comment

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

Clean! I love it.

@riederm riederm merged commit c186d0e into master Jun 12, 2024
13 checks passed
@riederm riederm deleted the mari/ast_visitor branch June 12, 2024 12:26
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.

4 participants