[PATCH] D41946: [clangd] Add support for different file URI schemas.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/URI.cpp:43
+      Body.consume_front("/");
+    return llvm::sys::path::convert_to_slash(Body);
+  }
----------------
sammccall wrote:
> Don't you want the opposite here, convert from unix to native?
Ah, right!


================
Comment at: clangd/URI.cpp:57
+      Body = "/";
+    Body += path::convert_to_slash(AbsolutePath, path::Style::posix);
+    return (llvm::Twine(Scheme) + ":" + percentEncode(Body)).str();
----------------
sammccall wrote:
> conversely, here you want native (which is the default)
Don't we still need to convert \ to / on windows?


================
Comment at: clangd/URI.h:29
+  llvm::StringRef scheme() const { return Scheme; }
+  /// \brief Returns decoded authority.
+  llvm::StringRef authority() const { return Authority; }
----------------
sammccall wrote:
> e.g. "//reviews.llvm.org"
I think authority doesn't include leading "//"? 


================
Comment at: clangd/URI.h:76
+  /// \brief Returns the absolute path of the file corresponding to the URI body
+  /// in the file system. \p CurrentFile is the file from which the request is
+  /// issued. This is needed because the same URI in different workspace may
----------------
sammccall wrote:
> I think "the file from which the request is issued" is overly-specified (and a little unclear).
> 
> For instance, consider this workflow:
>  vim $projectroot
>  go to symbol -> "MyClass" (from global index)
> Now we're trying to navigate to "monorepo://proj/myclass.h", but we don't have a "current file" to use as a hint. $projectroot would probably do nicely, and is available to clangd.
> 
> So maybe `llvm::StringRef HintPath`- "A related path, such as the current file or working directory, which can help disambiguate when the same file exists in many workspaces".
Makes sense. Thanks for the suggestion!


================
Comment at: clangd/URI.h:90
+/// - All other characters are escaped.
+std::string percentEncode(llvm::StringRef Content);
+
----------------
sammccall wrote:
> this seems easy enough to test via uri::create rather than exposing it publicly, but up to you.
I think this would also be useful for other scheme extensions. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41946