sammccall added inline comments.
================
Comment at: clangd/URI.h:63
+ /// Tries to get the include path of the file corresponding to the URI.
+ /// This allows clients to provide their customized include paths.
----------------
Avoid "tries to" which is vague about what cases are failures.
e.g.
Get the preferred spelling of this file for #include, if there is one, e.g.
<GL.h>.
Returns the empty string if normal include-shortening based on the path should
be used.
Fails if the URI is not valid in the schema.
================
Comment at: clangd/URI.h:64
+ /// Tries to get the include path of the file corresponding to the URI.
+ /// This allows clients to provide their customized include paths.
+ ///
----------------
what are "clients"? maybe "URI schemas"?
================
Comment at: clangd/URI.h:66
+ ///
+ /// If the returned string is non-empty, clangd will use it directly when
+ /// doing include insertion; otherwise we will fall back to the clang to
----------------
does this include <quotes>? it probably should, if we're allowing schemes to
customize how includes are spelled.
================
Comment at: clangd/URI.h:69
+ /// calculate the include path from the resolved absolute path.
+ static llvm::Expected<std::string> includePath(const URI &U);
+
----------------
i'd avoid the name "include path" because it can be confused with a) the set of
directories passed to -I and b) the filesystem path to the file to be included.
Suggest includeString or so
================
Comment at: clangd/URI.h:107
+ /// Returns the include path of the file corresponding to the URI, which can
+ /// be #include directly. See URI::resolveIncludePath for details.
+ virtual llvm::Expected<std::string>
----------------
#included
================
Comment at: unittests/clangd/ClangdTests.cpp:964
EXPECT_TRUE(Inserted("<y.h>", PreferredHeader, "\"Y.h\""));
+ auto TestURIHeader = URI::create("/abc/test-root/x/y/z.h", "unittest");
+ EXPECT_TRUE(static_cast<bool>(TestURIHeader));
----------------
this relies on ClangdTests and URITests being linked together, which we chose
to avoid last time this came up. Just define a scheme here?
(This is one place where putting a field on URI would have been simpler)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45426
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
