[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.

This is probably not a final version, but I feel my comments should be helpful anyway.
Also, we're missing testcases now.



================
Comment at: clangd/ClangdLSPServer.cpp:225
+  PathRef ref = PathRef((Params.uri.uri).c_str());
+  Path result = LangServer.Server.switchSourceHeader(ref);
+
----------------
We use `Params.uri.file`, not `Params.uri.uri` when passing paths to clangd.
Also no need for extra local var: `LangServer.Server.switchSourceHeader(Params.uri.file);` should work.


================
Comment at: clangd/ClangdLSPServer.cpp:225
+  PathRef ref = PathRef((Params.uri.uri).c_str());
+  Path result = LangServer.Server.switchSourceHeader(ref);
+
----------------
ilya-biryukov wrote:
> We use `Params.uri.file`, not `Params.uri.uri` when passing paths to clangd.
> Also no need for extra local var: `LangServer.Server.switchSourceHeader(Params.uri.file);` should work.
Naming: local vars should start with capital letter.


================
Comment at: clangd/ClangdServer.cpp:295
+  std::string DEFAULT_SOURCE_EXTENSIONS[] = { ".cpp", ".c", ".cc", ".cxx",
+    ".c++", ".C", ".m", ".mm" };  
+  std::string DEFAULT_HEADER_EXTENSIONS[] = { ".h", ".hh", ".hpp", ".hxx",
----------------
We should check all extensions in both upper-case and lower-case, not just `.c` and `.C`
(ideally, we could use case-insensitive comparisons).


================
Comment at: clangd/ClangdServer.cpp:302
+
+  std::string pathDataRef = std::string(path.data());
+  bool isSourceFile = false, foundExtension = false;
----------------
`path` is already a `StringRef`, no need to convert it. 
`std::string` is also implicitly convertible to `StringRef`, you could pass `std::string` to every function that accepts a `StringRef`


================
Comment at: clangd/ClangdServer.cpp:307
+
+  while (!foundExtension && i < sourceSize)
+  {
----------------
Maybe replace with:
```
if (std::find(DEFAULT_SOURCE_EXTESIONS, /*end iterator*/, llvm::sys::path::extensions(path)) {
  isSourceFile = true;
  foundExtension = true;
}
```


================
Comment at: clangd/ClangdServer.cpp:323
+  {
+    while (!foundExtension && i < headerSize)
+    {
----------------
Maybe use `std::find` here too?


================
Comment at: clangd/ClangdServer.cpp:339
+  SmallString<128> CurrentPath;
+  CurrentPath = temp;
+  bool done = false;
----------------
Don't need `temp` var here, there's `StringRef::toVector` method to convert `StringRef` to `SmallString`.


================
Comment at: clangd/ClangdServer.cpp:341
+  bool done = false;
+  std::vector<std::string> EXTENSIONS_ARRAY;
+  
----------------
Naming: EXTENSIONS_ARRAY is a local var, use `UpperCamelCase`.

Maybe use `ArrayRef<std::string>` for `ExtensionsArray` instead?


================
Comment at: clangd/ClangdServer.cpp:360
+    FILE * file;
+    file = fopen(temp.c_str(), "r");
+    if (file)
----------------
Please use `vfs::FileSystem` instance, provided by `FSProvider.getTaggedFileSystem()` here for file accesses.


================
Comment at: clangd/ClangdServer.cpp:367
+      std::string returnValue = std::string(NewPath.str());
+      return Path("\"" + returnValue + "\"");
+    }
----------------
Don't add quotes here, `Uri` constructor and `Uri::unparse` is responsible for doing that.


================
Comment at: clangd/ClangdServer.h:185
 
+  std::string switchSourceHeader(std::string path);
+
----------------
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Please use descriptive typedefs for paths: `Path` (equivalent to `std::string`) for return type and `PathRef` (equivalent to `StringRef`) for parameter type.
> Maybe add a small comment for this function to indicate it's a fairly trivial helper?
Maybe also change wording to indicate that this function does not mutate any state and is indeed trivial, i.e.:
`Helper function that returns a path to the corresponding source file when given a header file and vice versa.`

Code style: please end all comments with full stops.


================
Comment at: clangd/ClangdServer.h:186
+  // Helper function to change from one header file to it's corresponding source file and vice versa
+  Path switchSourceHeader(PathRef path);
+
----------------
Naming: parameter names must be capitalized


================
Comment at: clangd/ProtocolHandlers.cpp:260
+  Dispatcher.registerHandler(
+      "textDocument/switchSourceHeader",
+      llvm::make_unique<SwitchSourceHeaderHandler>(Out, Callbacks));
----------------
ilya-biryukov wrote:
> I don't see this method in the [[ https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md | LSP spec ]].
> Is this some kind of extension?
> 
> Could you add some comments on that with pointers to proposal/explanation of where this extension is used?
This comment was not addressed, probably marked as 'Done' by accident.


https://reviews.llvm.org/D36150