- 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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
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.
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. |
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. |
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. |