[PATCH] D42915: [clangd] Use URIs in index symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks OK so far, where is it going? It doesn't make sense to put URIs in if we only ever use `file:`.
I guess others will be produced by some kind of extension point on SymbolCollector. That will specify URI schemes we should try, allow you to replace the whole `toFileURI`, or something else?

Unfortunately there's a bunch of `Uri`s here, where the existing code uses `URI`...



================
Comment at: clangd/index/Index.h:27
+  // The URI of the source file where a symbol occurs.
+  llvm::StringRef FileUri;
   // The 0-based offset to the first character of the symbol from the beginning
----------------
nit: FileURI?
(The other style is OK too, though I personally find it harder to read. But the class is `URI` and we should be consistent)


================
Comment at: clangd/index/SymbolCollector.cpp:28
+// current working directory of the given SourceManager if the Path is not an
+// absolute path. If failed, this combine relative paths with \p FallbackDir to
+// get an absolute path.
----------------
this combines

or better, "resolves relative paths against \p FallbackDir"


================
Comment at: clangd/index/SymbolCollector.cpp:33
 // the SourceManager.
-std::string makeAbsolutePath(const SourceManager &SM, StringRef Path,
-                             StringRef FallbackDir) {
+std::string toFileUri(const SourceManager &SM, StringRef Path,
+                      StringRef FallbackDir) {
----------------
also URI here, and below


================
Comment at: clangd/index/SymbolCollector.cpp:201
+    std::string Uri;
+    S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir, Uri);
 
----------------
while here, would you mind changing GetSymbolLocation -> getSymbolLocation?


================
Comment at: clangd/index/SymbolYAML.cpp:51
     IO.mapRequired("EndOffset", Value.EndOffset);
-    IO.mapRequired("FilePath", Value.FilePath);
+    IO.mapRequired("FileUri", Value.FileUri);
   }
----------------
more URI


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42915