[PATCH] D147034: [clangd] Replace the hacky include-cleaner macro-reference implementation.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:143
+  std::vector<include_cleaner::SymbolReference> Macros;
+  for (const auto &MAndRefs : AST.getMacros().MacroRefs) {
+    for (const auto &Ref : MAndRefs.second) {
----------------
nit: `const auto &[SID, Refs]`


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:145
+    for (const auto &Ref : MAndRefs.second) {
+      auto L = sourceLocationInMainFile(SM, Ref.Rng.start);
+      if (!L) {
----------------
it's unfortunate that we're scanning the file for every occurrence of a macro just to get an offset. this might get problematic in big test files, what about introducing an offset to `MacroOccurence`?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:150
+      }
+      if (const auto *Tok =
+             AST.getTokens().spelledTokenAt(*L)) {
----------------
nit: early exit instead


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:156
+
+        if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid())
+          Macros.push_back(
----------------
we should use `Macro->NameLoc`, not the definition loc inside macro-info


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:162
+               Ref.InConditionalDirective
+                   ? include_cleaner::RefType::Implicit
+                   : include_cleaner::RefType::Explicit});
----------------
i believe you meant `Ambigious` here. the reference is "explicitly" in the code, but we can't say for sure if it's intended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147034