ioeric added inline comments.
================
Comment at: clangd/FileDistance.cpp:9
+//===----------------------------------------------------------------------===//
+//
+//
----------------
Is this intentionally reserved?
================
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);
----------------
nit: this probably deserve a comment or a better formatting. It looks like part
of the loop incremental.
================
Comment at: clangd/FileDistance.cpp:86
+ auto Canonical = canonicalize(Path);
+ unsigned Cost = kUnreachable;
+ SmallVector<hash_code, 16> Ancestors;
----------------
Should we also check the cache here?
================
Comment at: unittests/clangd/FileDistanceTests.cpp:36
+ // Ancestor (up+up+up+up)
+ EXPECT_EQ(D.distance("/"), 22u);
+ // Sibling (up+down)
----------------
sammccall wrote:
> ioeric wrote:
> > I think this is an interesting case. IIUC, 22u is contributed by 4 ups
> > (4*5) and 1 include (1*2, via `llvm/ADT/StringRef.h`).
> >
> > Suppose `a/b/c/d/e/f.cc` includes `base/x.h`, then the shortest path into
> > directory `other/` is likely to go via `base/x.h`. This might become a
> > problem if `base/x.h` is very commonly used. One solution is probably to
> > penalize edits that hit the root.
> Yeah, this could be a weakness. I don't think this is specific to #includes,
> or to root directories.
> I think the generalization is that there exist directories (like
> /usr/include) whose siblings are unrelated. Down-traversals in these
> directories should be more expensive. I've added a Caveats section to the
> FileDistance documentation, but I would like to avoid adding special cases
> until we can see how much difference they make.
As chatted offline, this could be still concerning as it's not uncommon for
files to include headers from top-level directories. I think the idea you
proposed - restricting up traversals from included files, could address the
problem.
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
