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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG with some nits addressed



================
Comment at: clangd/URI.cpp:138
+
+  auto Pos = Uri.find(':');
+  if (Pos == llvm::StringRef::npos)
----------------
ioeric wrote:
> sammccall wrote:
> > consider StringRef::split which avoids some index arithmetic:
> > 
> >   pair<StringRef, StringRef> SchemeSplit = Uri.split(':');
> >   if (SchemeSplit.second == "")
> >     return error
> >   U.Scheme = percentDecode(SchemeSplit.first);
> >   // then SchemeSplit.second contains the rest
> `split` is neat. But here we want to distinguish between `http:` vs `http` while it's not trivial wth `split`. Similar for `/` following the authority if we want to keep body '/', say, in `http://llvm.org/`.
I don't understand what you mean by distinguishing `http:` vs `http` - "http://foo" will split into {"http", "foo"}. "http//foo" will not split at all, and "http:://foo" will split into "http" and "://foo".

Agreed regarding authority/body.


================
Comment at: clangd/URI.cpp:46
+    llvm::SmallVector<char, 16> Path(Body.begin(), Body.end());
+    llvm::sys::path::native(Path);
+    return std::string(Path.begin(), Path.end());
----------------
should this be native(Path, posix)?
native(Path) seems to be a no-op


================
Comment at: clangd/URI.cpp:89
+  case '~':
+  case '/': // '/' is resereved, but we unescape it for readability.
+    return false;
----------------
nit: reserved
nit: you're not unescaping it, you're just not escaping it

and I don't think "for readability" is accurate.
Reserved characters are only reserved in certain contexts, escaping a slash in a path is (I think) incorrect.


================
Comment at: clangd/URI.h:31
+public:
+  /// \brief Returns decoded scheme e.g. "https"
+  llvm::StringRef scheme() const { return Scheme; }
----------------
nit: prefer to omit `\brief` unless you want the brief comment to be something other than the first sentence.

(I know we have this in lots  of places - it adds noise, and I finally got around to looking it up in the style guide!)


================
Comment at: clangd/URI.h:91
+/// \brief Encodes a string according to percent-encoding.
+/// - Unrerved characters are not escaped.
+/// - Reserved characters always escaped with exceptions like '/'.
----------------
nit: unreserved


================
Comment at: clangd/URI.h:82
+
+  virtual llvm::Expected<std::string>
+  uriFromAbsolutePath(llvm::StringRef AbsolutePath) const = 0;
----------------
sammccall wrote:
> similarly, this should return (authority, body), not just body.
> a pair seems fine for such an internal API
Still confused about this API, but let's chat offline?


================
Comment at: clangd/URI.h:90
+/// - All other characters are escaped.
+std::string percentEncode(llvm::StringRef Content);
+
----------------
ioeric wrote:
> 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. 
Hmm, I think if individual schemes need to do encoding/decoding, then our API is wrong :-)


================
Comment at: unittests/clangd/URITests.cpp:1
+//===-- URITests.cpp  ---------------------------------*- C++ -*-----------===//
+//
----------------
you may want a test that round-trips `getVirtualTestFilePath("x")` through a file URI and back again.
This should exercise the platform-specific code path - that path is under C:\ on windows, and /tmp/ on linux. The buildbots should give us platform coverage.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41946