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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/URI.cpp:94
+
+std::vector<std::string> createEncodeMap() {
+  std::vector<std::string> Result(128, "");
----------------
Performance doesn't really matter here, but it looks like this is intended to save time by precomputing, and I don't think it's any faster than the direct version of `percentEncode` which might be easier to read:

  for (unsigned char C : Content)
    if (shouldEscape(C))
      OS << '%' << format_hex_no_prefix(C, 2);
    else
      OS << C;

where shouldEscape just does some range checks.
(the "unsigned" is important!)


================
Comment at: clangd/URI.cpp:95
+std::vector<std::string> createEncodeMap() {
+  std::vector<std::string> Result(128, "");
+  for (char C : Unreserved)
----------------
128 should be 256, right?


================
Comment at: clangd/URI.cpp:123
+    }
+    if (I + 1 == E || I + 2 == E)
+      return make_string_error("Expect two characters after '%' sign: Content");
----------------
most implementations seem to just treat treat the % as literal in this case, e.g. "%41%" -> "A%", which makes this function unable to fail, which simplifies `parse()` a bit


================
Comment at: clangd/URI.cpp:125
+      return make_string_error("Expect two characters after '%' sign: Content");
+    char Buf[3];
+    Buf[0] = *(++I);
----------------
alternatively, using more llvm and less std:

  if (*I == '%' && I + 2 < Content.end()
    && isHexDigit(*(I+1)) && isHexDigit(*(I+2)) {
    Result.push_back(hexFromNibbles(*(I+1), *(I+2)));
    I += 2;
  } else
    Result.push_back(*I);
      


================
Comment at: clangd/URI.cpp:136
+  FileURI U;
+  llvm::StringRef OrigUri = Uri;
+
----------------
nit: i'd probably prefer to keep "uri" referring to the whole URI, and call the one you mutate something else


================
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


================
Comment at: clangd/URI.cpp:149
+    Pos = Uri.find('/');
+    if (Pos == llvm::StringRef::npos)
+      return make_string_error("Expect '/' after a URI authority: " + OrigUri);
----------------
this is e.g. `http://llvm.org`
This is valid: scheme = 'http', authority = 'llvm.org', path = ''.
(Wikipedia is wrong, but the RFC covers this)


================
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!)


================
Comment at: clangd/URI.cpp:168
+  if (U.Scheme.empty() || U.Body.empty())
+    return make_string_error("Scheme and body must be provided in URI: " +
+                             OrigUri);
----------------
body may be empty. (`http://llvm.org` again)


================
Comment at: unittests/clangd/URITests.cpp:115
+
+TEST(URITest, Parse) {
+  EXPECT_THAT(parseOrDie("file://auth//x/y/z"),
----------------
please include some typical examples:

"file:///etc/passwd"
"file:///c:/windows/system32/"
"http://llvm.org"
"urn:isbn:0451450523"


================
Comment at: unittests/clangd/URITests.cpp:117
+  EXPECT_THAT(parseOrDie("file://auth//x/y/z"),
+              AllOf(Scheme("file"), Authority("auth"), Body("/x/y/z")));
+
----------------
the body should be "//x/y/z" here - the / that terminates the authority is part of the path.
(This is a valid URI, but not a typical example)


================
Comment at: unittests/clangd/URITests.cpp:143
+  EXPECT_TRUE(FailedParse(":/a/b/c"));
+  EXPECT_TRUE(FailedParse("s:"));
+  // Incomplete.
----------------
this is the same as the next one


================
Comment at: unittests/clangd/URITests.cpp:144
+  EXPECT_TRUE(FailedParse("s:"));
+  // Incomplete.
+  EXPECT_TRUE(FailedParse("x:"));
----------------
these are both valid


================
Comment at: unittests/clangd/URITests.cpp:148
+  // Empty authority.
+  EXPECT_TRUE(FailedParse("file:////x/y/z"));
+}
----------------
this is valid, authority may be empty


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41946