sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/test/symbols-modules.test:1
+# Verify that we do not crash and correctly find the definition of a macro that
+# was imported from a module and then #undef-ed in preamble.
----------------
Generally, we prefer to write tests as gunit tests where feasible, this gathers
the inputs into one place, avoids too much messing around with the filesystem,
and assertions on something a little simpler than the full JSON output. It also
make it easier to factor out common bits across tests.
If I'm understanding this test right it should be fairly easy to port to a
gunit test with `TestTU` (setting AdditionalFiles and ExtraArgs) which would
assert on headerSymbols(). This would probably go in SymbolCollectorTests,
similar to e.g. `TEST_F(SymbolCollectorTest, NonModularHeader)`.
Obviously this relates to where other tests around indexing modules might live
once we have those. If you'd prefer them to be lit tests, it'd be nice to know
a bit about that.
================
Comment at: clang/include/clang/Lex/Preprocessor.h:1126
+ return MI;
+ MacroState &S = CurSubmoduleState->Macros[II];
+ auto ActiveModuleMacros = S.getActiveModuleMacros(*this, II);
----------------
This looks fairly grungy and I don't totally understand it :-(
I guess walking the getPrevious() chain on the macro definition until we hit
one with info doesn't work?
If we can't reuse something existing for this, we should probably ask rsmith if
this is sensible.
API quibbles:
- it seems this only differs for modules, but the name doesn't mention modules
- maybe this should just be a "bool LookIntoModules" in `getMacroInfo`? it has
only 3 callers.
================
Comment at: clang/lib/Index/IndexingAction.cpp:155
+ auto *MI = MD->getMacroInfo();
+ if (!MI) {
+ // It is possible for MI to be nullptr if we our translation unit had
----------------
You don't need to call getMacroInfo() here, getMacroInfoForLatestDefinition()
does this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80525/new/
https://reviews.llvm.org/D80525
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
