ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/URI.cpp:200
+ return make_string_error("Couldn't convert " + AbsolutePath +
+ " to any given scheme.");
+}
----------------
maybe also include the schemes in the error message. with `llvm::join`?
================
Comment at: clang-tools-extra/clangd/URI.h:48
+ /// Creates a URI for a file using the first valid scheme from \p Schemes.
+ /// \p Schemes entries must be registered. The URI is percent-encoded.
----------------
nit: `// Similar to above except this uses the first scheme in \p Schemes that
works.`
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:51
+ llvm::StringSet<> ParentURIs;
+ // Use ProximityPaths converted to URIs as proximity sources.
+ llvm::StringMap<SourceParams> RequestURIs;
----------------
this comment is no longer true.
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:52
+ // Use ProximityPaths converted to URIs as proximity sources.
+ llvm::StringMap<SourceParams> RequestURIs;
+ for (const auto &Path : ProximityPaths) {
----------------
These are not URIs anymore. Consider calling it `Sources`?
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:55
+ const auto PathProximityURIs =
+ generateProximityURIs(URI::create(Path, URISchemes)->toString());
+ if (PathProximityURIs.empty())
----------------
Result of `URI::create` must be explicitly checked; it would trigger assertion
if it is not checked or contains error.
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:58
+ continue;
+ RequestURIs[Path] = SourceParams();
+ for (const auto &ProximityURI : PathProximityURIs)
----------------
We should still add `Path` to `RequestURIs` even if there is no proximity URI.
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:65
+ SymbolRelevanceSignals PathProximitySignals;
+ // DistanceCalculator will find the shortest distance from requested URI to
+ // any URI extracted from the ProximityPaths.
----------------
nit: `s/request URI/ProximityPaths/`
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:242
+ auto ParsedURI = URI::parse(URIPath);
+ assert(ParsedURI &&
+ "Non-empty argument of generateProximityURIs() should be a valid "
----------------
I'd suggesting avoiding assertion here. Consider `elog` the error and returning
empty result.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:395
for (; !It.reachedEnd(); It.advance())
- Result.emplace_back(It.peek(), It.consume());
+ Result.emplace_back({It.peek(), It.consume()});
return Result;
----------------
Why do we need this change?
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:278
StaticIdx.reset(Placeholder = new
SwapIndex(llvm::make_unique<MemIndex>()));
- runAsync<void>([Placeholder] {
- if (auto Idx = loadIndex(YamlSymbolFile))
+ runAsync<void>([Placeholder, &Opts] {
+ if (auto Idx = loadIndex(YamlSymbolFile, Opts.URISchemes, UseDex))
----------------
(Have you rebased on HEAD? The code has been changed.)
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:623
+
+ auto I = DexIndex::build(std::move(Builder).build(), URISchemes);
+
----------------
kbobyrev wrote:
> ioeric wrote:
> > We could use the constructor that doesn't take ownership e.g.
> > `DexIndex({Sym1, Sym2}, URISchemes)`
> That would try to convert `{Sym1, Sym2}` to `SymbolSlab` and that's not
> possible. IIUC your suggestion is about using the constructor with ranges:
>
> ```
> template <typename Range>
> DexIndex(Range &&Symbols, llvm::ArrayRef<std::string> URISchemes)
> ```
>
> I could use `llvm::make_range(begin(Symbols), end(Symbols))`, but then I'd
> have to put everything into `Symbols` container first and that might be the
> same amount of code. I'm not sure that would be better than having a
> `Builder`. Did I miss something?
Have you tried std::vector<Symbol>{Sym1, Sym2}? or `push_back` twice? Using
`Builder` seems unnecessary but not too bad.
https://reviews.llvm.org/D51481
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
