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

[Eric Liu via Phabricator via cfe-commits]

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