[PATCH] D101388: [clangd] Add SymbolID to LocatedSymbol.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, but can you amend the description a bit?

> We do not have to both locate the symbol and also query the SymbolID (using getSymbolInfo).

IIRC this isn't really the reason, it's because getSymbolInfo and locateSymbolAt don't have the same semantics *for choosing symbols* and we want the ID of the symbol that matches the latter.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:260
       Macro.Definition = Loc;
+      Macro.ID = getSymbolID(M->Name, M->Info, AST.getSourceManager());
       return Macro;
----------------
note that this populates SymbolID even if there's no ID available, so it's incorrect if the type is Optional<SymbolID>


================
Comment at: clang-tools-extra/clangd/XRefs.h:51
   llvm::Optional<Location> Definition;
+  // SymbolID of the symbol. Not present for file referents.
+  llvm::Optional<SymbolID> ID;
----------------
There are other cases too, maybe just "if available"


================
Comment at: clang-tools-extra/clangd/XRefs.h:52
+  // SymbolID of the symbol. Not present for file referents.
+  llvm::Optional<SymbolID> ID;
 };
----------------
SymbolID is already inherently optional (it has a zero value for a whil enow)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:924
       EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
+      EXPECT_THAT(Results[0], HasID()) << Test;
       llvm::Optional<Range> GotDef;
----------------
nit, just `EXPECT_TRUE(Results[0].ID)`, and remove the matcher?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101388