[PATCH] D45426: [clangd] Allow using customized include path in URI.

[Sam McCall via Phabricator via cfe-commits]

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