Page MenuHomePhabricator

Fix the postorder visting of the ClassTemplateSpecializationDecl nodes in the RecursiveASTVisitor.
AcceptedPublic

Authored by MontyKutyi on Jun 8 2017, 5:14 AM.

Details

Summary

The TraverseDecl method of the RecursiveASTVisitor reaches different number of nodes in postorder than in preorder visit mode in some cases, which must be a bug.
In the following example during traversing the TranslationUnitDecl node the preorder visitor reaches the ClassTemplateSpecializationDecl node but the postorder visitor does not.

template <typename> class X
{};

template class X<int>;

This must be the result of a bug in the DEF_TRAVERSE_TMPL_SPEC_DECL, which returns true in some cases and preventing not only the traversal of the declaration context of that node but the calling of the postorder visit action in the DEF_TRAVERSE_DECL macro too.

Diff Detail

Event Timeline

MontyKutyi created this revision.Jun 8 2017, 5:14 AM
MontyKutyi removed rL LLVM as the repository for this revision.Jun 8 2017, 5:36 AM
MontyKutyi added a subscriber: cfe-commits.
MontyKutyi updated this revision to Diff 101929.Jun 8 2017, 8:40 AM

Just added the whole file instead of a small surrounding to be able to check the DEF_TRAVERSE_DECL macro too.

Could anybody take a look at on this please?

Thanks,
Peter

bruno added a subscriber: bruno.Jul 24 2017, 12:12 PM

Hi Peter,

Please add a testcase for this change, it also seems that you should update the comment.

Updated the comment.

As I can see the clang/test contains a lot of different simple tests, but for testing this I think it is not enough to run the clang with some arguments on a specific input. So I should create a new executable which uses the postorder mode of the RecursiveASTVisitor. Am I right or is there another preferred way for doing this?

bruno added a comment.Jul 25 2017, 9:56 AM

As I can see the clang/test contains a lot of different simple tests, but for testing this I think it is not enough to run the clang with some arguments on a specific input. So I should create a new executable which uses the postorder mode of the RecursiveASTVisitor. Am I right or is there another preferred way for doing this?

The place you're looking for here is in unittests. For example, see unittests/AST/PostOrderASTVisitor.cpp.

Added test for the fix.

Added test for the fix.

Great! The test code you added doesn't seem to be compatible with clang style, can you run clang format and update the test?

Is there any particular parameter for the clang-format what I should use? If I just run it without any parameter it changes lines of the original test too.

I run the clang-format with -style=llvm on the added code parts.

bruno added a comment.Jul 28 2017, 9:39 AM

Nice!

One more comment: please also fix names of local variables, member variables and function parameters. See https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly. Yes, there is precedence for violations in other places in this file, but that doesn't mean we should keep adding those :-)

Did the renamings.

Any further request on this?

MontyKutyi updated this revision to Diff 117367.Oct 2 2017, 9:21 AM

Updated to the latest trunk version.

bruno added a comment.Jan 4 2018, 3:00 PM

The change seems good to me in general. I wonder if this will hit any broken assumption in the code. Did you run other tests beside unittests?

rsmith accepted this revision.Jan 4 2018, 3:17 PM

Generally this looks good.

Is there some way we can prevent this bug from recurring by construction (eg, by making a return ill-formed, or making it "just work")? Wrapping the code block in the DEF_TRAVERSE_* invocation in a lambda would help, but would probably significantly increase the compile-time (and possibly code size) cost of RecursiveASTVisitor, so I'd prefer a different solution if possible. We could move the post-code-block actions into the destructor of an RAII object, but then we can't suppress running them when the code block returns false. Ideas welcome.

include/clang/AST/RecursiveASTVisitor.h
1587

Same problem here?

2430

Likewise.

This revision is now accepted and ready to land.Jan 4 2018, 3:17 PM

The change seems good to me in general. I wonder if this will hit any broken assumption in the code. Did you run other tests beside unittests?

I run the tests available by building the clang-test only.