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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
