[PATCH] D36150: [clangd] LSP extension to switch between source/header file

[Ilya Biryukov via Phabricator via cfe-commits]

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