ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Overall the code looks good, thanks for the clean-ups.
Only a few minor style comments and a major one about returning value when no
file is matching.
Please add tests for the new feature.
================
Comment at: clangd/ClangdLSPServer.cpp:227
+ TextDocumentIdentifier Params, StringRef ID, JSONOutput &Out) {
+
+ llvm::Optional<Path> Result =
----------------
Maybe remove this empty line?
================
Comment at: clangd/ClangdLSPServer.cpp:231
+ URI ResultUri;
+ if (Result != llvm::None)
+ ResultUri = URI::fromFile(*Result);
----------------
Replace with more concise form: `if (Result)`
================
Comment at: clangd/ClangdLSPServer.cpp:234
+ else
+ ResultUri = URI::fromFile("");
+
----------------
Running `unparse` on an instance created via `URI::fromFile("")` will result
in `"file:///"`.
Have you considered returning a list of paths as a result from
`switchHeaderSource`?
That way you could capture the "no file matched" case with an empty list.
Later when an implementation will look into index, that would allow to return
multiple source files for a header file, which also happens in some C++
projects.
Returning an empty string is also an option we could start with.
================
Comment at: clangd/ClangdServer.cpp:22
+using namespace llvm;
using namespace clang;
----------------
Do we really need this `using namespace` or it can be deleted?
================
Comment at: clangd/ClangdServer.cpp:345
+ if (FS->exists(NewPath)) {
+ return NewPath.str().str();
+ }
----------------
Code style: we don't use braces around small single-statement control structures
================
Comment at: clangd/ClangdServer.h:185
+ /// Helper function that returns a path to the corresponding source file
when given a header file and vice versa.
+ llvm::Optional<Path> switchSourceHeader(PathRef Path);
----------------
Could you please `clang-format` this file too?
Comment line length is clearly over the limit.
https://reviews.llvm.org/D36150
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
