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