sammccall added a comment.
Thanks for the changes! I still find the logic quite confusing. Happy to chat
offline if useful.
================
Comment at: clangd/XRefs.cpp:137
+
+IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
+ auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
----------------
this is a nice abstraction - consider hiding the DeclarationAndMacrosFinder
inside it!
================
Comment at: clangd/XRefs.cpp:215
- indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
- DeclMacrosFinder, IndexOpts);
+ // Handle goto definition for macros.
+ if (!Symbols.Macros.empty()) {
----------------
So now you're early-out if there are macros.
This means if you have
```
void log(string s) { cout << s; }
#define LOG log
LOG("hello");
```
You'll offer only line 2 as an option, and not line 1.
I'm not sure that's bad, but it's non-obvious - I think it's the thing that the
comment should call out.
e.g. "If the cursor is on a macro, go to the macro's definition without trying
to resolve the code it expands to"
(The current comment just echoes the code)
================
Comment at: clangd/XRefs.cpp:226
- std::vector<const Decl *> Decls = DeclMacrosFinder->takeDecls();
- std::vector<MacroDecl> MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ // Handle goto definition for typical declarations.
+ //
----------------
I'm not sure this line adds anything unless you are more specific about what
"typical" means, maybe just drop it.
(nit: go to, not goto)
================
Comment at: clangd/XRefs.cpp:233
+ // - We use the location from AST and index (if available) to provide the
+ // final results. Prefer AST over index.
+ //
----------------
because it's more up-to-date?
================
Comment at: clangd/XRefs.cpp:235
+ //
+ // - For each symbol, we will return a location of the cannonical
declaration
+ // (e.g. function declaration in header), and a location of definition if
----------------
nit: canonical
================
Comment at: clangd/XRefs.cpp:237
+ // (e.g. function declaration in header), and a location of definition if
+ // they are available, definition comes first.
+ struct CandidateLocation {
----------------
first why?
================
Comment at: clangd/XRefs.cpp:246
+
+ for (auto D : Symbols.Decls) {
+ llvm::SmallString<128> USR;
----------------
nit: the body of this loop handles building the query and getting the AST
locations, but they're interleaved in a confusing way - please separate them,
or even make these two separate loops.
In the latter case you could put QueryRequest inside the if(Index) block, which
would be easier to follow.
================
Comment at: clangd/XRefs.cpp:248
+ llvm::SmallString<128> USR;
+ if (index::generateUSRForDecl(D, USR))
+ continue;
----------------
why are we skipping the AST location if we can't generate a USR?
================
Comment at: clangd/XRefs.cpp:256
+ CandidateLocation &Candidate = ResultCandidates[ID];
+ bool IsDef = GetDefinition(D) == D;
+ // Populate one of the slots with location for the AST.
----------------
why do you not use GetDefinition(D) as the definition, in the case where it's
not equal to D?
================
Comment at: clangd/XRefs.cpp:265
+ if (Index) {
+ log("query index...");
+ std::string HintPath;
----------------
remove this log message or make it more useful :-)
================
Comment at: clangd/XRefs.cpp:277
+ // it.
+ auto ToLSPLocation = [&HintPath](
+ const SymbolLocation &Loc) -> llvm::Optional<Location>
{
----------------
(The double-nested lambda is somewhat hard to read, can this just be a
top-level function? That's what's needed to share it, right?)
================
Comment at: clangd/XRefs.cpp:516
SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
----------------
can you also use `getSymbolAtPosition` here?
================
Comment at: unittests/clangd/XRefsTests.cpp:42
// FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
- auto CI = createInvocationFromCommandLine(
- {"clang", "-xc++", testPath("Foo.cpp").c_str()});
- auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+ StringRef BaseName = "Main",
----------------
this is too complicated, please find a way to make it simpler.
Take a look in TestFS.h for the filesystem stuff.
================
Comment at: unittests/clangd/XRefsTests.cpp:43
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+ StringRef BaseName = "Main",
+ llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem>
----------------
why allow BaseName to be chosen?
================
Comment at: unittests/clangd/XRefsTests.cpp:44
+ StringRef BaseName = "Main",
+ llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem>
+ InMemoryFileSystem = nullptr) {
----------------
why do you need to pass in the FS? The FS shouldn't need to be the same between
the index and AST.
================
Comment at: unittests/clangd/XRefsTests.cpp:52
+ std::vector<const char *> Cmd = {"clang", "-xc++", MainPath.c_str()};
+ if (!HeaderCode.empty()) {
+ InMemoryFileSystem->addFile(HeaderPath, 0,
----------------
why do this conditionally?
================
Comment at: unittests/clangd/XRefsTests.cpp:72
+std::unique_ptr<SymbolIndex> buildIndexFromAST(ParsedAST *AST) {
+ SymbolCollector::Options CollectorOpts;
----------------
can we reuse FileIndex instead of reimplementing it?
================
Comment at: unittests/clangd/XRefsTests.cpp:162
+ // Build index.
+ auto IndexAST = build(SymbolCpp.code(), SymbolHeader.code(), "symbol",
+ InMemoryFileSystem);
----------------
combine these into `buildIndex`? Having multiple ASTs floating around in the
test seems confusing...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45717
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
