sammccall marked an inline comment as done.
sammccall added a comment.
Sigh... sorry, forgot to send these comments with the last patch. Will measure
performance today.
================
Comment at: clangd/FileDistance.cpp:9
+//===----------------------------------------------------------------------===//
+//
+//
----------------
ioeric wrote:
> Is this intentionally reserved?
Oops, added implementation docs
================
Comment at: clangd/FileDistance.cpp:51
+ for (StringRef Rest = Canonical; !Rest.empty();) {
+ Rest = parent_path(Rest, sys::path::Style::posix);
+ auto NextHash = hash_value(Rest);
----------------
ioeric wrote:
> nit: this probably deserve a comment or a better formatting. It looks like
> part of the loop incremental.
Moved out of the loop, now the loop has a counter. Hopefully less confusing?
================
Comment at: clangd/FileDistance.cpp:86
+ auto Canonical = canonicalize(Path);
+ unsigned Cost = kUnreachable;
+ SmallVector<hash_code, 16> Ancestors;
----------------
ioeric wrote:
> Should we also check the cache here?
I'm not sure why - for the full path we'll look up the cached value in the
first iteration through the loop. Ancestors will be empty, so this function
should be pretty cheap for this happy path - dominated by computing the hash.
or do you mean to skip canonicalization?
This doesn't matter in practice as we'll use this through URIDistance which
does a pre-canonicalization cache. If we used this directly then it might be
worthwhile.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48441
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
