[PATCH] D38425: [clangd] Document highlights for clangd

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.


================
Comment at: clangd/ClangdServer.cpp:528
+      Value = llvm::make_error<llvm::StringError>(
+              "invalid AST",
+              llvm::errc::invalid_argument);
----------------
Invalid


================
Comment at: clangd/ClangdServer.cpp:530
+              llvm::errc::invalid_argument);
+    Value = make_tagged(clangd::findDocumentHighlights(*AST, Pos, Logger), TaggedFS.Tag);
+  });
----------------
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.


================
Comment at: clangd/Protocol.cpp:372
+
+
+
----------------
Too many new lines. Only keep one.


================
Comment at: clangd/Protocol.h:568
+struct DocumentHighlight {
+  /*
+   *
----------------
Use /// like other places


================
Comment at: clangd/Protocol.h:575
+  /**
+   * The highlight kind, default is DocumentHighlightKind.Text.
+   */
----------------
Use /// like other places


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425