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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits