ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Just a few more comments. Should be easy to fix.
Could you also please `clang-format` the code?
================
Comment at: clangd/ClangdLSPServer.cpp:225
+ llvm::Optional<Path> Result =
LangServer.Server.switchSourceHeader(Params.uri.file);
+ Path ResultString = *Result;
+
----------------
What if `Result` does not contain a value?
================
Comment at: clangd/ClangdLSPServer.cpp:225
+ llvm::Optional<Path> Result =
LangServer.Server.switchSourceHeader(Params.uri.file);
+ Path ResultString = *Result;
+
----------------
ilya-biryukov wrote:
> What if `Result` does not contain a value?
Use `*Result` instead of introducing an extra variable?
================
Comment at: clangd/ClangdLSPServer.cpp:229
+ R"({"jsonrpc":"2.0","id":)" + ID.str() +
+ R"(,"result":)" + ResultString + R"(})");
+}
----------------
We should convert paths here. Convert to `URI` and use `unparse`.
```
Uri ResultUri = Uri::fromFile(*Result);
//....
R"(,"result":)" + Uri::unparse(ResultUri) + R"(})");
//...
```
================
Comment at: clangd/ClangdServer.cpp:325
+ // Lookup in a list of known extensions.
+ auto SourceIter = std::find(std::begin(SourceExtensions),
std::end(SourceExtensions), PathExt);
+ bool IsSource = SourceIter != std::end(SourceExtensions);
----------------
Instead of an `checkUpperCaseExtensions` and two iterations use a single
`find_if` and `equals_lower`:
```
// Checks for both uppercase and lowercase matches in a single loop over
extensions array.
std::find_if(std::begin(SourceExtensions), std::end(SourceExtensions),
[](PathRef SourceExt) {
return SourceExt.equals_lower(PathExt);
});
```
================
Comment at: clangd/ClangdServer.cpp:363
+ {
+ std::string FileHandle = "file://";
+ return "\"" + FileHandle + NewPath.str().str() + "\""; // First str() to
convert from SmallString to StringRef, second to convert from StringRef to
std::string
----------------
Don't add `"file://"` or quoting here, simply return `NewPath`.
================
Comment at: clangd/ClangdServer.h:186
+ /// If an extension hasn't been found in the lowercase array, try with
uppercase.
+ bool checkUpperCaseExtensions(StringRef BaseArray[], int ArraySize,
StringRef PathExt);
+
----------------
Any reason not to put it into anonymous namespace inside `.cpp` file?
================
Comment at: clangd/ProtocolHandlers.cpp:214
+ if (!TDPP) {
+ return;
+ }
----------------
Code style: we don't use braces around small single-statement control structures
https://reviews.llvm.org/D36150
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
