Skip to content

Conversation

mernst
Copy link
Contributor

@mernst mernst commented Jan 2, 2021

This pull request makes no logic changes, but it changes the visit order of ModifierVisitor.

ModifierVisitor works top-down, visiting larger then smaller scopes.

Many visit methods visit annotations and modiifers before members. This makes sense because the annotations and modifiers apply to the entire node; the entire node is within their scope. Examples of visit methods that work this way includes those for Parameter, ReceiverParameter, VariableDeclarationExpr, ArrayCreationLevel, VarType, PackageDeclaration, ModuleDeclaration, and some other node types.

In other cases, annotations and modifiers are visited later, or the two are visited at different places in the visit order. This pull request changes such instances.

This change was necessary for a visitor I wrote that needs to know which annotations each construct is in the scope of.

@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #3011 (c65f596) into master (f8b7bd6) will not change coverage.
The diff coverage is 47.619%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #3011   +/-   ##
=========================================
  Coverage   50.424%   50.424%           
=========================================
  Files          449       449           
  Lines        25067     25067           
  Branches      3790      3790           
=========================================
  Hits         12640     12640           
  Misses       11450     11450           
  Partials       977       977           
Flag Coverage Δ
AlsoSlowTests 50.424% <47.619%> (ø)
javaparser-core 50.424% <47.619%> (ø)
jdk-10 50.424% <47.619%> (ø)
jdk-11 50.424% <47.619%> (ø)
jdk-12 50.424% <47.619%> (ø)
jdk-13 50.424% <47.619%> (ø)
jdk-14 50.424% <47.619%> (ø)
jdk-15 50.424% <47.619%> (ø)
jdk-8 50.422% <47.619%> (ø)
jdk-9 50.424% <47.619%> (ø)
macos-latest 50.412% <47.619%> (ø)
ubuntu-latest 50.412% <47.619%> (ø)
windows-latest 50.412% <47.619%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...github/javaparser/ast/visitor/ModifierVisitor.java 28.709% <47.619%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8b7bd6...c65f596. Read the comment docs.

@MysterAitch
Copy link
Member

Seems reasonable - visit the annotations first, then members?

Perhaps this is something that we can formalise / make consistent throughout - do you see any other inconsistencies in the order that could be changed?

@mernst
Copy link
Contributor Author

mernst commented Jan 2, 2021

I didn't look for other inconsistencies. Other visitors could also be changed if desired. For example, HashCodeVisitor could be changed to put annotations and modifiers first, but that would not affect behavior since the hash codes are just added up.

@MysterAitch MysterAitch merged commit 1349a79 into javaparser:master Jan 2, 2021
@MysterAitch MysterAitch added this to the next release milestone Jan 2, 2021
@mernst mernst deleted the modifiervisitor-top-down branch January 2, 2021 20:47
@MysterAitch
Copy link
Member

Ah... I just realised that the visitor is generated (missing the @Generated annotation) which means the ordering gets reset/overridden whenever the generators are run next.

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