Page MenuHomePhabricator

[clangd] Document highlights for clangd
ClosedPublic

Authored by Nebiroth on Sep 29 2017, 1:28 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
malaperle added inline comments.Nov 27 2017, 12:50 PM
clangd/ClangdUnit.cpp
1029 ↗(On Diff #124391)

Last

1069 ↗(On Diff #124391)

Don't call getLocForEndOfToken here again. This is the reason why sometimes it includes an extra character. Just to explain a bit, supposed we have
int Foo;

the first call to getLocForEnd in handleDeclOccurence will compute the position at
int Foo|;
Then if you call is again, it will go at the end of the ';' token so it will now be
int Foo;|

This revision now requires changes to proceed.Nov 27 2017, 12:50 PM
Nebiroth updated this revision to Diff 124458.Nov 27 2017, 1:33 PM
Nebiroth edited edge metadata.
Nebiroth marked 3 inline comments as done.

Minor code cleanup
Fixed highlights sometimes obtaining one too many characters inside range
Updated test

malaperle added inline comments.Nov 27 2017, 1:34 PM
clangd/ClangdLSPServer.cpp
246 ↗(On Diff #124391)

I get a test failure here because there is an assertion that the Expected<> needs to be checked. I can't really think of any failure case right now where we wouldn't just return an empty array of highlights. But I think it's better for consistency and future-proofing to keep the Expected<>.
I think you can just do like in onRename for now

if (!Highlights) {
  C.replyError(ErrorCode::InternalError,
               llvm::toString(Highlights.takeError()));
  return;
}
malaperle added inline comments.Nov 28 2017, 1:32 PM
clangd/ClangdLSPServer.cpp
246 ↗(On Diff #124391)

Just to be clear, the error I see in the output is:
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

It asserts when in ClangdLSPServer::onDocumentHighlight, referencing Highlights->Value

Nebiroth updated this revision to Diff 124810.Nov 29 2017, 1:44 PM

Added verification for llvm::Expected in onDocumentHighlight
Removed unused variable in ClangdUnit

Nebiroth updated this revision to Diff 124834.Nov 29 2017, 3:30 PM

Fixed wrong content header making the test fail

ilya-biryukov requested changes to this revision.Nov 30 2017, 1:31 AM
ilya-biryukov added inline comments.
clangd/ClangdLSPServer.cpp
67 ↗(On Diff #123848)

Still there

clangd/ClangdServer.cpp
513 ↗(On Diff #124834)

Other functions don't crash in this case anymore, we should follow the same pattern here (see findDefinitions for an example)

524 ↗(On Diff #124834)

We should return an error this case.

clangd/ClangdUnit.cpp
1062 ↗(On Diff #124834)

This function should be private.

1096 ↗(On Diff #124834)

We should not return default-initialized Locations from this function.
Return llvm::Optional<Location> and handle the case when getFileEntryForID returns null by returning llvm::None.

1165 ↗(On Diff #124834)

Let's add a comment that we don't currently handle macros. It's ok for the first iteration, having documentHighlights for Decls is useful enough to get it in.

clangd/Protocol.h
621 ↗(On Diff #124834)

Please also compare kind here, to make this operator consistent with operator==.
std::tie(LHS.range, LHS.kind) < std::tie(RHS.range, RHS.kind) should work. If it doesn't, converting enums to int should help: std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, int(RHS.kind))

test/clangd/initialize-params-invalid.test
23 ↗(On Diff #124834)

NIT: Misindent

test/clangd/initialize-params.test
23 ↗(On Diff #124834)

NIT: Misindent

This revision now requires changes to proceed.Nov 30 2017, 1:31 AM
Nebiroth updated this revision to Diff 124951.Nov 30 2017, 8:51 AM
Nebiroth edited edge metadata.
Nebiroth marked 6 inline comments as done.
Minor code cleanup
getDeclarationLocation now returns llvm::Optional
operator< for DocumentHighlight struct now properly compares the kind field
malaperle requested changes to this revision.Nov 30 2017, 12:08 PM

very minor comments

clangd/ClangdServer.h
288 ↗(On Diff #124951)

"for a symbol hovered on."

It doesn't have to be a symbol and the user doesn't have to hover on it. So maybe just "for a given position"

clangd/ClangdUnit.cpp
1088 ↗(On Diff #124951)
const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
if (!F)
  return llvm::None;
1098 ↗(On Diff #124951)

maybe move the check earlier? see comment above.

clangd/Protocol.h
601 ↗(On Diff #124951)

Use /// like other structs

This revision now requires changes to proceed.Nov 30 2017, 12:08 PM
malaperle added inline comments.Nov 30 2017, 12:53 PM
clangd/ClangdServer.cpp
526 ↗(On Diff #124951)

missing return?
I get a warning that looks legit:
./tools/clang/tools/extra/clangd/ClangdServer.cpp:526:7: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]

llvm::make_error<llvm::StringError>(
Nebiroth updated this revision to Diff 125228.Dec 1 2017, 2:54 PM
Nebiroth edited edge metadata.

Minor code cleanup
unparse and parse methods for JSON are updated

malaperle requested changes to this revision.Dec 3 2017, 9:08 PM
malaperle added inline comments.
clangd/ClangdServer.cpp
528 ↗(On Diff #125228)

Invalid

530 ↗(On Diff #125228)

It trips here with assertions enabled because we haven't checked the previous "Value" for error before, sigh.
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

How about this:

std::vector<DocumentHighlight> Result;
llvm::Optional<llvm::Error> Err;
Resources->getAST().get()->runUnderLock([Pos, &Result, &Err,
                                         this](ParsedAST *AST) {
  if (!AST)
    Err = llvm::make_error<llvm::StringError>("Invalid AST",
                                              llvm::errc::invalid_argument);
  Result = clangd::findDocumentHighlights(*AST, Pos, Logger);
});

if (Err)
  return std::move(*Err);

return make_tagged(Result, TaggedFS.Tag);

The tests pass for me with this.

clangd/Protocol.cpp
372 ↗(On Diff #125228)

Too many new lines. Only keep one.

clangd/Protocol.h
568 ↗(On Diff #125228)

Use /// like other places

575 ↗(On Diff #125228)

Use /// like other places

This revision now requires changes to proceed.Dec 3 2017, 9:08 PM
Nebiroth updated this revision to Diff 125346.Dec 4 2017, 8:11 AM
Nebiroth edited edge metadata.

Minor code cleanup
Refactored findDocumentHighlights() to make tests pass when assertions are enabled

Can you also reformat the code too with clang-format? I think there's a few things in ClangdUnit.h/.cpp

clangd/ClangdServer.cpp
521 ↗(On Diff #125346)

We should do like findDefinitions here:

if (!Resources)
  return llvm::make_error<llvm::StringError>(
      "findDocumentHighlights called on non-added file",
      llvm::errc::invalid_argument);
528 ↗(On Diff #125346)

Woops, I forgot the return here if it's an error, so it should be:

if (!AST) {
  Err = llvm::make_error<llvm::StringError>("Invalid AST",
                                            llvm::errc::invalid_argument);
  return;
}
malaperle requested changes to this revision.Dec 4 2017, 8:47 PM
This revision now requires changes to proceed.Dec 4 2017, 8:47 PM
Nebiroth updated this revision to Diff 125522.Dec 5 2017, 6:58 AM
Nebiroth edited edge metadata.

Added error returns in findDocumentHighlights
Ran clang-format on ClangdUnit.h, .cpp and ClangdServer.cpp

malaperle requested changes to this revision.Dec 5 2017, 8:27 AM

Sorry I forgot to submit this additional comment in my last review pass :(

clangd/Protocol.cpp
365 ↗(On Diff #125346)

static_cast<int>(DH.kind)

This revision now requires changes to proceed.Dec 5 2017, 8:27 AM
Nebiroth updated this revision to Diff 125619.Dec 5 2017, 2:44 PM
Nebiroth edited edge metadata.

Added static_cast when unparsing

malaperle accepted this revision.Dec 5 2017, 8:58 PM
malaperle added a subscriber: sammccall.

This looks good to me. @ilya-biryukov, @sammccall Any objections?
I think we can do further iterations later to handle macros and other specific cases (multiple Decls at the position, etc) . It's already very useful functionality.

Overall looks good, just a few minor comments.
Let me know if need someone to land this patch for you after you fix those.

clangd/ClangdUnit.cpp
249 ↗(On Diff #125619)

Mentioning AST in this comment is weird, macros have nothing to do with the AST. We should remove/rephrase the comment.
I'm not sure if multiple occurences of MacroInfo are even possible here, but we could leave the code as is anyway.

clangd/Protocol.h
562 ↗(On Diff #125619)

NIT: remove this empty comment line and all the others.

581 ↗(On Diff #125619)

This comparison does not provide a total order.
Please use std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, int(RHS.kind)) instead.

ilya-biryukov requested changes to this revision.Dec 8 2017, 4:19 AM
This revision now requires changes to proceed.Dec 8 2017, 4:19 AM
Nebiroth updated this revision to Diff 126371.Dec 11 2017, 8:19 AM
Nebiroth edited edge metadata.
Nebiroth marked 3 inline comments as done.

Merged with latest llvm/clang
Minor code cleanup

@ilya-biryukov Need someone to land this.

ilya-biryukov accepted this revision.EditedDec 12 2017, 4:24 AM

LGTM module a slight tweak to fix compilation. I'll land this today.

This revision is now accepted and ready to land.Dec 12 2017, 4:24 AM
Closed by commit rL320474: [clangd] Document highlights for clangd (authored by ibiryukov, committed by ). · Explain WhyDec 12 2017, 4:28 AM
This revision was automatically updated to reflect the committed changes.