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

DetailAST should invalidate childCount cache #3486

Closed
rnveach opened this Issue Oct 1, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Oct 1, 2016

Similar to #3466 (comment) and the existing issue described at #3466 (comment), we must erase the cache stored inside DetailAST between the non-comment trees and comment trees as the extra nodes being added make the cache values wrong.

The fix in the previous issue invalidated branchTokenTypes cache but it did not invalidate childCount which was also in issue's description.
Since we also cache childCount and don't reset it on add/set children/siblings methods, it will keep count from non-comment trees into comment trees, as long as we don't call setFirstChild which is the only method to invalidate it currently. To avoid calling setFirstChild, issue can be seen with code samples like public /* comment */ static void method () {} where a comment/javadoc is between 2 other similar nodes.

In previous issue's fix, getChildCount was called on METHOD_DEF in UT while the comments are located as a child of MODIFIERS in input. So the UT in that fix isn't directed at the correct location to showcase the getChildCount issue.

Showcase of issue at: #3485 (comment)

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 1, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 1, 2016

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Oct 1, 2016

Contributor

@romani

public /* comment */ static void method () {}

That is correct test case and we did not check it when we were discussing the issue #3466. @rnveach is right we should reset childCount cache.

Contributor

MEZk commented Oct 1, 2016

@romani

public /* comment */ static void method () {}

That is correct test case and we did not check it when we were discussing the issue #3466. @rnveach is right we should reset childCount cache.

romani added a commit that referenced this issue Oct 2, 2016

@romani romani added this to the 7.2 milestone Oct 2, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 2, 2016

Member

Fix is merged

Member

romani commented Oct 2, 2016

Fix is merged

@romani romani closed this Oct 2, 2016

agcuda added a commit to agcuda/checkstyle that referenced this issue Oct 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment