[PATCH] D53312: [clangd] Support limiting down traversals from sources in FileDistance.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The slightly weird down-traversals semantics are indeed slightly weird. I'm happy to move forward with this approach though if we document it.

I did think of one alternative:

- instead of `unsigned Source::MaxDownTraversals`, have `bool Source::AllowDownTraversals` (obviously less flexible)
- instead of `(min-cost, max-down-traversals)` in the cache, store `(first=min-cost, second=min-cost-allowing-downtraversals)`.

When populating tree based on sources, set both cache values if down-traversals are allowed, otherwise just the first.
When propagating scores from parent to child, only read from the second cache value, and write to both.
When looking up a result from the cache, use the first cached value.

I don't think there's a lot of practical difference for our use case, the advantage would be the semantics are easier to reason about. WDYT?



================
Comment at: clangd/FileDistance.cpp:84
+        Node.Cost = S.getValue().Cost + I * Opts.UpCost;
+        if (SourceParams::MaxTraversals - S.getValue().MaxDownTraversals >= I)
+          Node.MaxDownTraversals = S.getValue().MaxDownTraversals + I;
----------------
I don't understand this if condition


================
Comment at: clangd/FileDistance.cpp:90
+          // reachable with a higher cost but large down traversal limit.
+          if (Node.Cost < R.first->second.Cost) {
+            R.first->second = std::move(Node);
----------------
For consistency, tiebreak with down traversals: `|| Node.Cost == R.first->second.Cost && Node.MaxDownTraversals > R.first->second.MaxDownTraversals` (note the opposite sign)

But probably clearer to write this by defining `CacheNode::operator<`


================
Comment at: clangd/FileDistance.cpp:107
   while (!Next.empty()) {
-    auto ParentCost = Cache.lookup(Next.front());
-    for (auto Child : DownEdges.lookup(Next.front())) {
-      auto &ChildCost =
-          Cache.try_emplace(Child, Unreachable).first->getSecond();
-      if (ParentCost + Opts.DownCost < ChildCost)
-        ChildCost = ParentCost + Opts.DownCost;
-      Next.push(Child);
+    const auto &Parent = Cache[Next.front()];
+    if (Parent.MaxDownTraversals > 0) {
----------------
This fills in the cache if it wasn't set - it's probably harmless but not quite as clear. Does lookup not work here?


================
Comment at: clangd/FileDistance.h:65
+  // Limits the number of upwards/downwards traversals allowed from this source.
+  static constexpr unsigned MaxTraversals =
+      std::numeric_limits<unsigned>::max();
----------------
nit: "unlimited" might be a clearer name here.


================
Comment at: clangd/FileDistance.h:68
+  unsigned MaxUpTraversals = MaxTraversals;
+  unsigned MaxDownTraversals = MaxTraversals;
 };
----------------
We should document the slightly weird semantics.


================
Comment at: unittests/clangd/FileDistanceTests.cpp:124
+  // These are actually reacahble from "/a" with high cost. But we don't support
+  // such case.
+  EXPECT_EQ(D.distance("/a/b"), FileDistance::Unreachable);
----------------
, because /a itself is reached more cheaply via the root.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53312