[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Essentially looks good to me now, thanks!



================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:789
+          else
+            OS << "<anonymous> ";
+          D->getSourceRange().print(OS,
----------------
njames93 wrote:
> aaron.ballman wrote:
> > Should this be `"<anonymous> : "` instead?
> Good catch
It'd probably be good to add some test coverage for these weird edge cases (unnamed structs, constructors or other special member functions without a name).


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:796
+                                    MV.ActiveASTContext->getSourceManager());
+        } else if (const auto *T = Item.second.get<Type>()) {
+          OS << T->getTypeClassName() << "Type : ";
----------------
njames93 wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > Do we also need a match for `TypeLoc` matchers, or does the `else` cover that sufficiently well?
> > > > 
> > > > (Actually, should we handle all of the various matchers at: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/ASTMatchers/ASTMatchers.h#L141 rather than leaving it to an `else`? Then the `else` can become an unreachable so we know to update this interface?)
> > > The else should be sufficient for most general cases, the only reason some are special cased is to improve the output, but I don't want there to be a burden to update this interface if new nodes are added.
> > I should verify: does this map hold arbitrary AST nodes, or does the map only hold the top-level classes in the AST matching hierarchy?
> > 
> > If it's arbitrary AST nodes, then yeah, I definitely agree the `else` here is fine. If it's top-level classes instead, we don't add those all that often and so it doesn't seem like a major burden to keep those in sync.
> It can hold any kind of node that can be stored in a DynTypedNode, so essentially it's any arbitrary AST node.
Thanks for clarifying that; the `else` look *beautiful* to me! :-D


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120185/new/

https://reviews.llvm.org/D120185