Page MenuHomePhabricator

[clangd] Document highlights for clangd
ClosedPublic

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

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
316

Last

356

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

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

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

Still there

clangd/ClangdServer.cpp
515

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

526

We should return an error this case.

clangd/ClangdUnit.cpp
349

This function should be private.

384

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.

467

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
582

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

NIT: Misindent

test/clangd/initialize-params.test
23

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
289

"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
375
const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
if (!F)
  return llvm::None;
385

maybe move the check earlier? see comment above.

clangd/Protocol.h
562

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
528

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
530

Invalid

532

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

Too many new lines. Only keep one.

clangd/Protocol.h
568

Use /// like other places

575

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
523

We should do like findDefinitions here:

if (!Resources)
  return llvm::make_error<llvm::StringError>(
      "findDocumentHighlights called on non-added file",
      llvm::errc::invalid_argument);
530

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

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

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

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

581

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.