ioeric added inline comments.
================
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:
> ioeric wrote:
> > sammccall wrote:
> > > conversely, here you want native (which is the default)
> > Don't we still need to convert \ to / on windows?
> yes. `convert_to_slash(path, style)` assumes that `path` is in `style`.
>
> `convert_to_slash("c:\\foo.txt", windows)` --> "c:/foo.txt"
> `convert_to_slash("c:\\foo.txt", posix)` --> "c:\\foo.txt" (because that's a
> perfectly valid filename on a posix system! well, apart from the colon)
> `convert_to_slash("c:\\foo.txt", native)` --> "c:/foo.txt" if the host is
> windows, "c:\\foo.txt" if the host is windows (this is what you want)
>
I see. Thanks for the clarification!
================
Comment at: clangd/URI.cpp:138
+
+ auto Pos = Uri.find(':');
+ if (Pos == llvm::StringRef::npos)
----------------
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/`.
================
Comment at: clangd/URI.cpp:155
+ return Decoded.takeError();
+ if (Decoded->empty())
+ return make_string_error(
----------------
sammccall wrote:
> this is e.g. `file:///foo/bar`, which is definitely valid!
> scheme = 'file', authority = '', path = '/foo/bar'
>
> (we should have a test for this one!)
Right! Realized `file:////foo/bar` was weird while preparing the previous diff
;)
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
