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

Wed Jan 17 11:25:10 PST 2018

sammccall added a comment.

Few more interface nits, sorry for picking here! Will get through the impl in the morning.

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

================
Comment at: clangd/URI.cpp:50
+
+    if (!AbsolutePath.startswith("/"))
+      return make_string_error(
----------------
this error can be returned by the framework already (it's valid for all paths)

================
Comment at: clangd/URI.cpp:57
+      Body = "/";
+    Body += path::convert_to_slash(AbsolutePath, path::Style::posix);
+    return (llvm::Twine(Scheme) + ":" + percentEncode(Body)).str();
----------------
conversely, here you want native (which is the default)

================
Comment at: clangd/URI.cpp:58
+    Body += path::convert_to_slash(AbsolutePath, path::Style::posix);
+    return (llvm::Twine(Scheme) + ":" + percentEncode(Body)).str();
+  }
----------------
hmm, file:///foo/bar is more widely used than file:/foo/bar.
You way want to emit that instead?

================
Comment at: clangd/URI.h:24
+/// service might expose repo:// URIs that are relative to the source control
+/// root.
+class FileURI {
----------------
You probably also want to describe roughly the subset of URI parsing we do.

e.g.
Clangd handles URIs of the form <scheme>:[//<authority>]<body>.
It doesn't further split the authority or body into constituent parts (e.g. query strings is included in the body).

================
Comment at: clangd/URI.h:27
+public:
+  /// \brief Returns decoded scheme.
+  llvm::StringRef scheme() const { return Scheme; }
----------------
e.g. "https"

================
Comment at: clangd/URI.h:29
+  llvm::StringRef scheme() const { return Scheme; }
+  /// \brief Returns decoded authority.
+  llvm::StringRef authority() const { return Authority; }
----------------
e.g. "//reviews.llvm.org"

================
Comment at: clangd/URI.h:31
+  llvm::StringRef authority() const { return Authority; }
+  /// \brief Returns decoded body.
+  llvm::StringRef body() const { return Body; }
----------------
e.g. "/D41946"

================
Comment at: clangd/URI.h:43
+
+  /// \brief Resolves the absolute path of \p U with the first matching scheme
+  /// registered.
----------------
I think "the first matching scheme" is a bit confusing - I guess here "scheme" refers to URIScheme, but in practice these should be 1:1 with schemes.
I'd just drop that, and instead add a sentence that errors may occur if the scheme isn't understood or the URI isn't valid in the scheme.

================
Comment at: clangd/URI.h:46
+  static llvm::Expected<std::string> resolve(const FileURI &U,
+                                             llvm::StringRef CurrentFile = "");
+
----------------
this is the "more public" API, so the semantics of CurrentFile should probably be defined here, and merely referred to below.
(See below comment for questions about semantics)

================
Comment at: clangd/URI.h:63
+/// custom URI scheme. This is expected to be implemented and exposed via the
+/// URISchemeRegistry. Users are not expected to use URIScheme directly.
+///
----------------
Hmm - what does "users" mean :-)
I can't think of a good way to rephrase. It might be clear enough without this sentence.

================
Comment at: clangd/URI.h:65
+///
+/// Different codebases/projects can have different file schemes, and clangd
+/// interprets a file path according to the scheme. For example, a file path
----------------
nit: I'm not sure this longer comment would really help someone use this API.
I'd suggest either making the example concrete and focusing around that, or dropping it entirely - it's OK to be a little sparse here, since we don't really expect unfamiliar users.

================
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
----------------
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".

================
Comment at: clangd/URI.h:80
+  virtual llvm::Expected<std::string>
+  getAbsolutePath(llvm::StringRef Body, llvm::StringRef CurrentFile) const = 0;
+
----------------
You should probably pass authority here too, or we're committing all schemes to ignore authority

================
Comment at: clangd/URI.h:82
+
+  virtual llvm::Expected<std::string>
+  uriFromAbsolutePath(llvm::StringRef AbsolutePath) const = 0;
----------------
similarly, this should return (authority, body), not just body.
a pair seems fine for such an internal API

================
Comment at: clangd/URI.h:90
+/// - All other characters are escaped.
+std::string percentEncode(llvm::StringRef Content);
+
----------------
this seems easy enough to test via uri::create rather than exposing it publicly, but up to you.

================
Comment at: clangd/URI.h:97
+/// in the file system.
+typedef llvm::Registry<URIScheme> URISchemeRegistry;
+
----------------
nit: you probably don't want anything between URIScheme and this - if you keep percent*, move them below?

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41946