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