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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.


================
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
----------------
sammccall wrote:
> does this include <quotes>? it probably should, if we're allowing schemes to customize how includes are spelled.
Yes, the returned string is a literal string quoted with either <> or "". 


================
Comment at: clangd/URI.h:69
+  /// calculate the include path from the resolved absolute path.
+  static llvm::Expected<std::string> includePath(const URI &U);
+
----------------
sammccall wrote:
> 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
Renamed it to `spellingInclude`.


================
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));
----------------
sammccall wrote:
> 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)
I think we probably want a scheme for unittest purpose (we also have one for lit test), but this case only needs a trivial one, added here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45426