sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
A few more nits (and some ideas for further restructuring the merge logic).
Otherwise LG, let's land this!
================
Comment at: clangd/XRefs.cpp:215
- indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
- DeclMacrosFinder, IndexOpts);
+ // Handle goto definition for macros.
+ if (!Symbols.Macros.empty()) {
----------------
hokein wrote:
> sammccall wrote:
> > 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)
> This is a non-functional change.
>
> For the above example, only line 2 will be offered. This is expected behavior
> IMO -- when we go to definition on a macro, users would expect the macro
> definition.
Ah, so these cases can't both happen (we even have an assert elsewhere).
Still, we can treat them as independent, and it seems simpler: you can remove
the if statement, the comment for the if statement, and the early return.
================
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 {
----------------
hokein wrote:
> sammccall wrote:
> > first why?
> because this is `go to definition`, so it is sensible to return the
> `definition` as the first result, right?
Again, I don't think "definition" in LSP is being used in its technical c++
sense.
No issue with the behavior here, but I think the comment should be "we think
that's what the user wants most often" or something - it's good to know the
reasoning in case we want to change the behavior in the future.
================
Comment at: clangd/XRefs.cpp:277
+ // it.
+ auto ToLSPLocation = [&HintPath](
+ const SymbolLocation &Loc) -> llvm::Optional<Location>
{
----------------
hokein wrote:
> sammccall wrote:
> > (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?)
> Moved it to `XRef.h`, and also replace the one in `findsymbols`.
This is pretty weird in terms of layering I think :-(
The function is pretty trivial, but depends on a bunch of random stuff.
Can we move it back (and live with the duplication) until we come up with a
good home?
================
Comment at: clangd/XRefs.cpp:244
+ // final results. When there are duplicate results, we prefer AST over
+ // index because AST is more up-to-update.
+ //
----------------
up-to-update -> up-to-date
================
Comment at: clangd/XRefs.cpp:257
+ // 4. Return all populated locations for all symbols, definition first.
+ struct CandidateLocation {
+ llvm::Optional<SymbolID> ID;
----------------
OK, as discussed offline the bit that remains confusing is why this isn't a map.
The reason is that some symbols don't have USRs and therefore no symbol IDs.
In practice we don't really expect multiple symbol results at all, so what
about one of:
- `map<Optional<SymbolID>, CandidateLocations>`
- `map<SymbolID, CandidateLocations>` and use `SymbolID("")` as a sentinel
I slightly prefer the latter because it minimizes the invasiveness of handling
this rare/unimportant case.
================
Comment at: clangd/index/FileIndex.h:26
+/// \brief Retrieves namespace and class level symbols in \p AST.
+SymbolSlab indexAST(ParsedAST *AST);
----------------
nit: prefer to remove `\brief` in favor of formatting the comment so the
summary gets extracted.
================
Comment at: clangd/index/FileIndex.h:26
+/// \brief Retrieves namespace and class level symbols in \p AST.
+SymbolSlab indexAST(ParsedAST *AST);
----------------
sammccall wrote:
> nit: prefer to remove `\brief` in favor of formatting the comment so the
> summary gets extracted.
This isn't the main interface of the file.
Move to the bottom, and note that this is exposed to assist in unit tests?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45717
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
