[PATCH] D80525: [clangd] Fix crash-bug in preamble indexing when using modules.

Mon May 25 11:47:03 PDT 2020

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