[clangd] Store offsets in MacroOccurrence

[hokein]

Summary

Remove the existing Rng field.

From the review comment: https://reviews.llvm.org/D147034

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/CollectMacros.cpp

37–38

nit: SM.getFileOffset

clang-tools-extra/clangd/CollectMacros.h

26–29

Represents an occurrence in main-file sounds like the documentation for the struct not for these fields.
what about // Half-open range (end offset is exclusive) inside the main file.. can you also move EndOffset to its own line?

[hokein]

address comments

clang-tools-extra/clangd/CollectMacros.h

 
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "index/SymbolID.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
+#include <cstddef>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
 struct MacroOccurrence {
-  // Instead of storing SourceLocation, we have to store the token range because
-  // SourceManager from preamble is not available when we build the AST.
-  Range Rng;
+  // Half-open range (end offset is exclusive) inside the main file.
+  size_t StartOffset;
+  size_t EndOffset;
+
   bool IsDefinition;
   // True if the occurence is used in a conditional directive, e.g. #ifdef MACRO
   bool InConditionalDirective;
+
+  Range toRange(const SourceManager &SM) const;
 };
 
 struct MainFileMacros {
   llvm::StringSet<> Names;
   llvm::DenseMap<SymbolID, std::vector<MacroOccurrence>> MacroRefs;
   // Somtimes it is not possible to compute the SymbolID for the Macro, e.g. a
   // reference to an undefined macro. Store them separately, e.g. for semantic
   // highlighting.

clang-tools-extra/clangd/CollectMacros.cpp

 //===--- CollectMacros.cpp ---------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CollectMacros.h"
 #include "AST.h"
+#include "Protocol.h"
+#include "SourceCode.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/STLExtras.h"
+#include <cstddef>
 
 namespace clang {
 namespace clangd {
 
+Range MacroOccurrence::toRange(const SourceManager &SM) const {
+  auto MainFile = SM.getMainFileID();
+  return halfOpenToRange(
+      SM, syntax::FileRange(MainFile, StartOffset, EndOffset).toCharRange(SM));
+}
+
 void CollectMainFileMacros::add(const Token &MacroNameTok, const MacroInfo *MI,
                                 bool IsDefinition, bool InIfCondition) {
   if (!InMainFile)
     return;
   auto Loc = MacroNameTok.getLocation();
   if (Loc.isInvalid() || Loc.isMacroID())
     return;
 
   auto Name = MacroNameTok.getIdentifierInfo()->getName();
   Out.Names.insert(Name);
-  auto Range = halfOpenToRange(
-      SM, CharSourceRange::getCharRange(Loc, MacroNameTok.getEndLoc()));
+  size_t Start = SM.getFileOffset(Loc);
+  size_t End = SM.getFileOffset(MacroNameTok.getEndLoc());
   if (auto SID = getSymbolID(Name, MI, SM))
-    Out.MacroRefs[SID].push_back({Range, IsDefinition, InIfCondition});
+    Out.MacroRefs[SID].push_back({Start, End, IsDefinition, InIfCondition});
   else
-    Out.UnknownMacros.push_back({Range, IsDefinition, InIfCondition});
+    Out.UnknownMacros.push_back({Start, End, IsDefinition, InIfCondition});
 }
 
 void CollectMainFileMacros::FileChanged(SourceLocation Loc, FileChangeReason,
                                         SrcMgr::CharacteristicKind, FileID) {
   InMainFile = isInsideMainFile(Loc, SM);
 }
 
 void CollectMainFileMacros::MacroExpands(const Token &MacroName,

clang-tools-extra/clangd/SemanticHighlighting.cpp

findExplicitReferences(
             if (isUniqueDefinition(Decl))
               Tok.addModifier(HighlightingModifier::Definition);
           }
         }
       },
       AST.getHeuristicResolver());
   // Add highlightings for macro references.
   auto AddMacro = [&](const MacroOccurrence &M) {
-    auto &T = Builder.addToken(M.Rng, HighlightingKind::Macro);
+    auto &T = Builder.addToken(M.toRange(C.getSourceManager()),
+                               HighlightingKind::Macro);
     T.addModifier(HighlightingModifier::GlobalScope);
     if (M.IsDefinition)
       T.addModifier(HighlightingModifier::Declaration);
   };
   for (const auto &SIDToRefs : AST.getMacros().MacroRefs)
     for (const auto &M : SIDToRefs.second)
       AddMacro(M);
   for (const auto &M : AST.getMacros().UnknownMacros)

clang-tools-extra/clangd/XRefs.cpp

if (Macro) {
     // Handle references to macro.
     if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
       // Collect macro references from main file.
       const auto &IDToRefs = AST.getMacros().MacroRefs;
       auto Refs = IDToRefs.find(MacroSID);
       if (Refs != IDToRefs.end()) {
         for (const auto &Ref : Refs->second) {
           ReferencesResult::Reference Result;
-          Result.Loc.range = Ref.Rng;
+          Result.Loc.range = Ref.toRange(SM);
           Result.Loc.uri = URIMainFile;
           if (Ref.IsDefinition) {
             Result.Attributes |= ReferencesResult::Declaration;
             Result.Attributes |= ReferencesResult::Definition;
           }
           Results.References.push_back(std::move(Result));
         }
       }

clang-tools-extra/clangd/index/SymbolCollector.cpp

void SymbolCollector::handleMacros(const MainFileMacros &MacroRefsToIndex) {
   const auto &SM = PP->getSourceManager();
   const auto MainFileEntryRef = SM.getFileEntryRefForID(SM.getMainFileID());
   assert(MainFileEntryRef);
 
   const std::string &MainFileURI = HeaderFileURIs->toURI(*MainFileEntryRef);
   // Add macro references.
   for (const auto &IDToRefs : MacroRefsToIndex.MacroRefs) {
     for (const auto &MacroRef : IDToRefs.second) {
-      const auto &Range = MacroRef.Rng;
+      const auto &Range = MacroRef.toRange(SM);
       bool IsDefinition = MacroRef.IsDefinition;
       Ref R;
       R.Location.Start.setLine(Range.start.line);
       R.Location.Start.setColumn(Range.start.character);
       R.Location.End.setLine(Range.end.line);
       R.Location.End.setColumn(Range.end.character);
       R.Location.FileURI = MainFileURI.c_str();
       R.Kind = IsDefinition ? RefKind::Definition : RefKind::Reference;

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

 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using testing::UnorderedElementsAreArray;
 
-MATCHER_P(rangeIs, R, "") { return arg.Rng == R; }
+MATCHER_P(rangeIs, R, "") {
+  return arg.StartOffset == R.Begin && arg.EndOffset == R.End;
+}
 MATCHER(isDef, "") { return arg.IsDefinition; }
 MATCHER(inConditionalDirective, "") { return arg.InConditionalDirective; }
 
 TEST(CollectMainFileMacros, SelectedMacros) {
   // References of the same symbol must have the ranges with the same
   // name(integer). If there are N different symbols then they must be named
   // from 1 to N. Macros for which SymbolID cannot be computed must be named
   // "Unknown". The payload of the annotation describes the extra bit
const char *Tests[] = {
         int D = $2[[PREPEND]](A);
       )cpp",
       R"cpp(
         #define $1(def)[[MACRO_ARGS2]](X, Y) X Y
         #define $3(def)[[BAR]] 1
         #define $2(def)[[FOO]] $3[[BAR]]
         int A = $2[[FOO]];
       )cpp"};
-  auto ExpectedResults = [](const Annotations &T, StringRef Name) {
+  auto ExpectedResults = [](const llvm::Annotations &T, StringRef Name) {
     std::vector<Matcher<MacroOccurrence>> ExpectedLocations;
     for (const auto &[R, Bits] : T.rangesWithPayload(Name)) {
       if (Bits == "def")
         ExpectedLocations.push_back(testing::AllOf(rangeIs(R), isDef()));
       else if (Bits == "condit")
         ExpectedLocations.push_back(
             testing::AllOf(rangeIs(R), inConditionalDirective()));
       else
         ExpectedLocations.push_back(testing::AllOf(rangeIs(R)));
     }
     return ExpectedLocations;
   };
 
   for (const char *Test : Tests) {
-    Annotations T(Test);
+    llvm::Annotations T(Test);
     auto Inputs = TestTU::withCode(T.code());
     Inputs.ExtraArgs.push_back("-std=c++2b");
     auto AST = Inputs.build();
     auto ActualMacroRefs = AST.getMacros();
     auto &SM = AST.getSourceManager();
     auto &PP = AST.getPreprocessor();
     for (const auto &[Name, Ranges] : T.all_ranges()) {
       if (Name == "Unknown") {

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

 //===-- ParsedASTTests.cpp ------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // These tests cover clangd's logic to build a TU, which generally uses the APIs
 // in ParsedAST and Preamble, via the TestTU helper.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../../clang-tidy/ClangTidyCheck.h"
 #include "AST.h"
-#include "Annotations.h"
 #include "Compiler.h"
 #include "Config.h"
 #include "Diagnostics.h"
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "Preamble.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "TidyProvider.h"
 #include "support/Context.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Testing/Annotations/Annotations.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <utility>
 
 namespace clang {
 namespace clangd {
TEST(ParsedASTTest, CanBuildInvocationWithUnknownArgs) {
 
   // Unknown forwarded to -cc1 should not a failure either.
   Inputs.CompileCommand.CommandLine = {
       "clang", "-Xclang", "-fsome-unknown-flag", testPath("foo.cpp")};
   EXPECT_NE(buildCompilerInvocation(Inputs, IgnoreDiags), nullptr);
 }
 
 TEST(ParsedASTTest, CollectsMainFileMacroExpansions) {
-  Annotations TestCase(R"cpp(
+  llvm::Annotations TestCase(R"cpp(
     #define ^MACRO_ARGS(X, Y) X Y
     // - preamble ends
     ^ID(int A);
     // Macro arguments included.
     ^MACRO_ARGS(^MACRO_ARGS(^MACRO_EXP(int), E), ^ID(= 2));
 
     // Macro names inside other macros not included.
     #define ^MACRO_ARGS2(X, Y) X Y
TU.HeaderCode = R"cpp(
     MACRO_EXP(int B);
   )cpp";
   TU.AdditionalFiles["foo.inc"] = R"cpp(
     int C = ID(1);
     #define DEF 1
     int D = DEF;
   )cpp";
   ParsedAST AST = TU.build();
-  std::vector<Position> MacroExpansionPositions;
+  std::vector<size_t> MacroExpansionPositions;
   for (const auto &SIDToRefs : AST.getMacros().MacroRefs) {
     for (const auto &R : SIDToRefs.second)
-      MacroExpansionPositions.push_back(R.Rng.start);
+      MacroExpansionPositions.push_back(R.StartOffset);
   }
   for (const auto &R : AST.getMacros().UnknownMacros)
-    MacroExpansionPositions.push_back(R.Rng.start);
-  EXPECT_THAT(MacroExpansionPositions,
-              testing::UnorderedElementsAreArray(TestCase.points()));
+    MacroExpansionPositions.push_back(R.StartOffset);
+  EXPECT_THAT(
+      MacroExpansionPositions,
 }
 
 MATCHER_P(withFileName, Inc, "") { return arg.FileName == Inc; }
 
 TEST(ParsedASTTest, PatchesAdditionalIncludes) {
   llvm::StringLiteral ModifiedContents = R"cpp(
     #include "baz.h"
     #include "foo.h"

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

 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Testing/Annotations/Annotations.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest-matchers.h"
 #include "gtest/gtest.h"
 #include <memory>
 #include <optional>
 #include <string>
 #include <utility>
 #include <vector>

 #line 3
 #define         BAR
 )cpp",
       },
   };
 
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Contents);
-    Annotations Modified(Case.Contents);
+    llvm::Annotations Modified(Case.Contents);
     EXPECT_THAT(getPreamblePatch("", Modified.code()),
                 MatchesRegex(Case.ExpectedPatch));
 
     auto AST = createPatchedAST("", Modified.code());
     ASSERT_TRUE(AST);
-    std::vector<Range> MacroRefRanges;
+    std::vector<llvm::Annotations::Range> MacroRefRanges;
     for (auto &M : AST->getMacros().MacroRefs) {
       for (auto &O : M.getSecond())
-        MacroRefRanges.push_back(O.Rng);
+        MacroRefRanges.push_back({O.StartOffset, O.EndOffset});
     }
     EXPECT_THAT(MacroRefRanges, Contains(Modified.range()));
   }
 }
 
 TEST(PreamblePatchTest, OrderingPreserved) {
   llvm::StringLiteral Baseline = "#define BAR(X) X";
   Annotations Modified(R"cpp(