[PATCH] D44882: [clangd] Implementation of workspace/symbol request

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:258
+        if (!Items)
+          return replyError(ErrorCode::InvalidParams,
+                            llvm::toString(Items.takeError()));
----------------
`InvalidParams` doesn't match all cases where internal errors occur. Maybe use `ErrorCode::InternalError`?


================
Comment at: clangd/FindSymbols.cpp:145
+    if (!Uri) {
+      log(llvm::formatv(
+          "Workspace symbol: Could not parse URI '{0}' for symbol '{1}'.",
----------------
I think we may want to report the error to client instead of just logging them.


================
Comment at: clangd/SourceCode.cpp:143
+/// From "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no
+/// qualifier.
+std::pair<llvm::StringRef, llvm::StringRef>
----------------
nit: this comment is duplicated with the one in header.


================
Comment at: clangd/SourceCode.h:59
+/// Turn a pair of offsets into a Location.
+llvm::Optional<Location>
+offsetRangeToLocation(const DraftStore &DS, SourceManager &SourceMgr,
----------------
We should use `llvm::Expected`?

The function needs a comment documenting its behavior, especially on the unsaved file content. 


================
Comment at: clangd/SourceCode.h:61
+offsetRangeToLocation(const DraftStore &DS, SourceManager &SourceMgr,
+                      StringRef File, size_t OffsetStart, size_t OffsetEnd);
+
----------------
nit: name `FilePath` or `FileName`, `File` seems to be a bit confusing, does it mean `FileContent` or `FileName`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44882