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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
