[PATCH] D41946: [clangd] Add support for different file URI schemas.
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 20 08:17:30 PST 2018
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();
> 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)
> 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(
> 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 ;)
rCTE Clang Tools Extra
More information about the cfe-commits