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