[PATCH] D24862: Workaround ASTMatcher crashes. Added some more test cases.

[Eric Liu via cfe-commits]

ioeric added a comment.

Thanks for the comments!


================
Comment at: change-namespace/ChangeNamespace.cpp:279
@@ -276,2 +278,3 @@
   Finder->addMatcher(
-      usingDecl(hasAnyUsingShadowDecl(IsInMovedNs)).bind("using_decl"), this);
+      usingDecl(IsInMovedNs, hasAnyUsingShadowDecl(decl())).bind("using_decl"),
+      this);
----------------
aaron.ballman wrote:
> Why is this change an improvement even after the underlying bug is fixed?
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.

================
Comment at: change-namespace/ChangeNamespace.cpp:296
@@ -291,5 +295,3 @@
   // definitions, so we need to exclude them in the callback handler.
-  auto FuncMatcher = functionDecl(
-      hasParent(namespaceDecl()),
-      unless(anyOf(IsInMovedNs, hasAncestor(namespaceDecl(isAnonymous())),
-                   hasAncestor(cxxRecordDecl()))));
+  // NOTE: ASTMatcher crash when `FunctionDecl` is a lambda defined in parameter
+  // list, in which case it has no parent map. Workaround by filtering out
----------------
aaron.ballman wrote:
> 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.
Fair enough. I think I'll just get rid of the comment regarding crash and let the unittest handle it.

================
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:73
@@ -71,2 +72,3 @@
 int main(int argc, const char **argv) {
+  sys::PrintStackTraceOnErrorSignal(argv[0]);
   tooling::CommonOptionsParser OptionsParser(argc, argv,
----------------
aaron.ballman wrote:
> I don't think these changes are necessary as part of this patch.
Removed.


https://reviews.llvm.org/D24862