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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Great, this all makes sense. I think we can/should make the scheme selection a bit more robust (we shouldn't crash if we get unexpected filenames).
And... Uri or URI (I really think this is a usability issue - i had a scarring experience with a codebase that couldn't decide how to spell abbreviations...)



================
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
----------------
ioeric wrote:
> sammccall wrote:
> > 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)
> I tend to use `Uri` for URI strings and `URI`/`U` for URI objects.  But I'd be okay with either, if you prefer `URI`. 
I'm fine with either spelling, but not really with mixing the two.

Is there some precedent for using different capitalization depending on the variable type? That seems... non obvious to me.


================
Comment at: clangd/index/SymbolCollector.cpp:67
+  if (!U)
+    llvm_unreachable(llvm::toString(U.takeError()).c_str());
+  return U->toString();
----------------
this doesn't seem unreachable?


================
Comment at: clangd/index/SymbolCollector.h:38
     std::string FallbackDir;
+    /// Specifies the URI scheme used to encode file paths in symbols.
+    std::string FileURIScheme = "file";
----------------
We should document the semantics/exceptions in case of failure.
Do we drop the symbol, or drop the location, or fall back to file URI?

One scheme that would be flexible and I think pretty simple:
 - make this a vector of schemes that will be tried in order
 - drop the location (but not symbol) if it's not possible to encode the path
 - note that the vector should probably end in "file" as a fallback, and make `{"file"}` the default
For our monorepo we can set this to {"google", "file"} or just {"google"} if we never want to leak local paths. It seems pretty plausible that other orgs whose builds aren't totally hermetic would want several schemes here.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:49
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(Uri, P, "") { return arg.CanonicalDeclaration.FileUri == P; }
 MATCHER_P(LocationOffsets, Offsets, "") {
----------------
while renaming this, maybe Decl is better? we'll have Def soon!


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:195
+#ifndef LLVM_ON_WIN32
+TEST_F(SymbolCollectorTest, CustomURIScheme) {
+  CollectorOpts.IndexMainFiles = false;
----------------
Can you add a test for the case when the path fails to convert to the URI scheme?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42915