This is an archive of the discontinued LLVM Phabricator instance.

Workaround ASTMatcher crashes. Added some more test cases.
ClosedPublic

Authored by ioeric on Sep 23 2016, 6:34 AM.

Details

Summary
  • UsingDecl matcher crashed when UsingShadowDecl has no parent map. Workaround by moving parent check into UsingDecl.
  • FunctionDecl matcher crashed when there is a lambda defined in parameter list (also due to no parent map). Workaround by putting unless(cxxMethodDecl()) before parent check.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric updated this revision to Diff 72268.Sep 23 2016, 6:34 AM
ioeric retitled this revision from to Workaround ASTMatcher crashes. Added some more test cases..
ioeric updated this object.
ioeric added a reviewer: hokein.
ioeric added a subscriber: cfe-commits.
ioeric updated this revision to Diff 72269.Sep 23 2016, 6:37 AM
  • Update comments
aaron.ballman added a subscriber: aaron.ballman.

Should this perhaps be fixed in the AST matchers rather than in the check itself?

Should this perhaps be fixed in the AST matchers rather than in the check itself?

I'll file bugs for the crashes, but the fix in this patch is not that "dirty" - it simply changes the order of matchers to workaround the crashes. So I guess this doesn't need to wait for the fix in matcher library.

Should this perhaps be fixed in the AST matchers rather than in the check itself?

I'll file bugs for the crashes, but the fix in this patch is not that "dirty" - it simply changes the order of matchers to workaround the crashes. So I guess this doesn't need to wait for the fix in matcher library.

Whenever possible, we should be fixing the underlying bugs, not working around them.

ioeric updated this revision to Diff 72273.Sep 23 2016, 7:14 AM
  • Update comments.

Should this perhaps be fixed in the AST matchers rather than in the check itself?

I'll file bugs for the crashes, but the fix in this patch is not that "dirty" - it simply changes the order of matchers to workaround the crashes. So I guess this doesn't need to wait for the fix in matcher library.

Whenever possible, we should be fixing the underlying bugs, not working around them.

Acked, and I totally agree with you :) It's just that the change in this patch would still be valid after the underlying bugs are fixed, so I thought it was fine to fix those bugs after this.

Acked, and I totally agree with you :) It's just that the change in this patch would still be valid after the underlying bugs are fixed, so I thought it was fine to fix those bugs after this.

I might be confused then (this happens from time to time) -- are the changes in this patch still necessary after the underlying bugs are fixed?

ioeric updated this revision to Diff 72275.Sep 23 2016, 7:27 AM
  • Update comment.

Acked, and I totally agree with you :) It's just that the change in this patch would still be valid after the underlying bugs are fixed, so I thought it was fine to fix those bugs after this.

I might be confused then (this happens from time to time) -- are the changes in this patch still necessary after the underlying bugs are fixed?

Sorry for the confusion. I guess I should've been clearer in the comments and patch summary... The changes would've been what we wanted even without the underlying bugs and would not be reverted after the bugs are fixed.

Sorry for the confusion. I guess I should've been clearer in the comments and patch summary... The changes would've been what we wanted even without the underlying bugs and would not be reverted after the bugs are fixed.

Ah, thank you for clarifying! Now it makes more sense to me. :-)

Actual review comments are inline.

change-namespace/ChangeNamespace.cpp
279 ↗(On Diff #72275)

Why is this change an improvement even after the underlying bug is fixed?

296 ↗(On Diff #72275)

Comment is fine, but it should be reworded once the underlying bug is fixed, which makes me wonder if the comment is really adding value. Might make more sense to not talk in terms of the crash, but just why you need to filter this way.

change-namespace/tool/ClangChangeNamespace.cpp
73 ↗(On Diff #72275)

I don't think these changes are necessary as part of this patch.

unittests/change-namespace/ChangeNamespaceTests.cpp
325 ↗(On Diff #72275)

May want to duplicate this FIXME to where we are generating the change?

350 ↗(On Diff #72275)

Same here as above.

ioeric updated this revision to Diff 72284.Sep 23 2016, 8:06 AM
ioeric marked 3 inline comments as done.
  • Addressed review comments.

Thanks for the comments!

change-namespace/ChangeNamespace.cpp
279 ↗(On Diff #72275)

I think IsInMovedNs makes more sense to be a filter for using decl, and it is also more restrictive than hasAnyUsingShadowDecl, which would make the failure matching stop earlier IMO.

296 ↗(On Diff #72275)

Fair enough. I think I'll just get rid of the comment regarding crash and let the unittest handle it.

change-namespace/tool/ClangChangeNamespace.cpp
73 ↗(On Diff #72275)

Removed.

aaron.ballman accepted this revision.Sep 26 2016, 7:21 AM
aaron.ballman added a reviewer: aaron.ballman.

LGTM, thank you!

change-namespace/ChangeNamespace.cpp
279–281 ↗(On Diff #72284)

I had originally thought that this was changing behavior, but I am starting to think this is actually fine.

This revision is now accepted and ready to land.Sep 26 2016, 7:21 AM
hokein accepted this revision.Sep 26 2016, 11:08 AM
hokein edited edge metadata.

LGTM.

This revision was automatically updated to reflect the committed changes.