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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits