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

[hokein]

Summary

Now MainFileMacros preserves enough information, we perform a just-in-time
convertion to interop with include-cleaner::Macro for include-cleaer features.

[hokein]

Friendly ping in case you missed it.

[kadircet]

thanks!

clang-tools-extra/clangd/IncludeCleaner.cpp

143

nit: const auto &[SID, Refs]

145

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?

150

nit: early exit instead

156

we should use Macro->NameLoc, not the definition loc inside macro-info

162

i believe you meant Ambigious here. the reference is "explicitly" in the code, but we can't say for sure if it's intended.

[hokein]

address comments

[hokein]

thanks for the comments.

clang-tools-extra/clangd/IncludeCleaner.cpp

145

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?

145

thanks, it makes sense, sent out https://reviews.llvm.org/D153259.

156

oh, right. NameLoc takes the preamble patch into account.

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

303

nit: auto Loc = SM.getComposedLoc(MainFile, Offset)

312

nit: do early exit here as well

[hokein]

address comments

clang-tools-extra/clangd/IncludeCleaner.cpp

static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
   if (isFilteredByConfig(Cfg, Inc.Resolved)) {
     dlog("{0} header is filtered out by the configuration", FE->getName());
     return false;
   }
   return true;
 }
 
 std::vector<include_cleaner::SymbolReference>
-collectMacroReferences(ParsedAST &AST) {
+convertMacroReferences(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
-  //  FIXME: !!this is a hacky way to collect macro references.
-  std::vector<include_cleaner::SymbolReference> Macros;
   auto &PP = AST.getPreprocessor();
-  for (const syntax::Token &Tok :
-       AST.getTokens().spelledTokens(SM.getMainFileID())) {
-    auto Macro = locateMacroAt(Tok, PP);
+  std::vector<include_cleaner::SymbolReference> Macros;
+  for (const auto &MAndRefs : AST.getMacros().MacroRefs) {
+    for (const auto &Ref : MAndRefs.second) {
+      auto L = sourceLocationInMainFile(SM, Ref.Rng.start);
+      if (!L) {
+        llvm::consumeError(L.takeError());
+        continue;
+      }
+      if (const auto *Tok =
+             AST.getTokens().spelledTokenAt(*L)) {
+        auto Macro = locateMacroAt(*Tok, PP);
         if (!Macro)
           continue;
+
         if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid())
           Macros.push_back(
-          {Tok.location(),
-           include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
-                                  DefLoc},
-           include_cleaner::RefType::Explicit});
+              {Tok->location(),
+               include_cleaner::Macro{
+                   /*Name=*/PP.getIdentifierInfo(Tok->text(SM)), DefLoc},
+               Ref.InConditionalDirective
+                   ? include_cleaner::RefType::Implicit
+                   : include_cleaner::RefType::Explicit});
+      }
+    }
   }
   return Macros;
 }
 
 llvm::StringRef getResolvedPath(const include_cleaner::Header &SymProvider) {
   switch (SymProvider.kind()) {
   case include_cleaner::Header::Physical:
     return SymProvider.physical()->tryGetRealPathName();
for (const auto *Inc : UnusedIncludes) {
     F.Edits.back().range.end.line = Inc->HashLine + 1;
   }
   return Result;
 }
 } // namespace
 
 include_cleaner::Includes
 convertIncludes(const SourceManager &SM,
                 const llvm::ArrayRef<Inclusion> Includes) {
   include_cleaner::Includes ConvertedIncludes;
   for (const Inclusion &Inc : Includes) {
     include_cleaner::Include TransformedInc;
     llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written);
     TransformedInc.Spelled = WrittenRef.trim("\"<>");
     TransformedInc.HashLocation =
         SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset);
     TransformedInc.Line = Inc.HashLine + 1;
     TransformedInc.Angled = WrittenRef.starts_with("<");
     auto FE = SM.getFileManager().getFile(Inc.Resolved);
     if (!FE) {
       elog("IncludeCleaner: Failed to get an entry for resolved path {0}: {1}",
            Inc.Resolved, FE.getError().message());
       continue;
     }
     TransformedInc.Resolved = *FE;
     ConvertedIncludes.add(std::move(TransformedInc));

 IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
   const auto &Includes = AST.getIncludeStructure();
   include_cleaner::Includes ConvertedIncludes =
       convertIncludes(SM, Includes.MainFileIncludes);
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
 
   std::vector<include_cleaner::SymbolReference> Macros =
-      collectMacroReferences(AST);
+      convertMacroReferences(AST);
   std::vector<MissingIncludeDiagInfo> MissingIncludes;
   llvm::DenseSet<IncludeStructure::HeaderID> Used;
   trace::Span Tracer("include_cleaner::walkUsed");
   include_cleaner::walkUsed(
       AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
       AST.getPragmaIncludes(), SM,
       [&](const include_cleaner::SymbolReference &Ref,
           llvm::ArrayRef<include_cleaner::Header> Providers) {

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

 $insert_d[[]]$insert_foo[[]]#include "fuzz.h"
 #include "header.h"
 $insert_foobar[[]]#include <e.h>
 $insert_f[[]]$insert_vector[[]]
 
 #define DEF(X) const Foo *X;
 #define BAZ(X) const X x
 
+// No missing include insertion for ambiguous macro refs.
+#if defined(FOO)
+#endif
+
   void foo() {
     $b[[b]]();
 
     ns::$bar[[Bar]] bar;
     bar.d();
     $f[[f]](); 
 
     // this should not be diagnosed, because it's ignored in the config