[PATCH] D88411: [clangd] Introduce MemoryTrees

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:28
+    size_t ChildrenSize = 0;
+    for (const auto &C : Children) {
+      C.traverseTree(CollapseDetails,
----------------
kadircet wrote:
> sammccall wrote:
> > Here the detailed nodes are entirely collapsed. This isn't quite as powerful as collapsing the detail *edges* only, which is worth considering.
> > 
> > e.g. consider TUScheduler. The natural accounting of resources is (IMO)
> > ```
> > clangd.TUScheduler.<filename>.AST
> > clangd.TUScheduler.<filename>.Preamble.InMemory
> > clangd.TUScheduler.<filename>.Preamble.Inputs
> > ```
> > or something like that.
> > 
> > Instead of aggregating to `clangd.TUScheduler` only, collapsing the `<filename>` edges mean you get `clangd.TUScheduler.AST` etc.
> yes this was an option i hadn't considered. my only hesitation is this likely implies different paths with the same name. current api also doesn't protect against it, but they are a lot less likely as we collapse whole subtrees.
> 
> I believe deleting the edge should also imply aggregating all the paths with the same name, e.g.
> `a.b.<deleted-1>.c, 5` and `a.b.<deleted-2>.c, 8` should end up being `a.b.c,13`. WDYT?
Agree with your desired behavior.

If you're willing to make collapsing a property of the tree rather than the query, you avoid this problem, because you collapse up-front and make addChild idempotent:

```
class Tree {
  BumpPtrAllocator *DetailAllocator; // Null if no detail
  StringMap<Tree> Children;
  size_t Self = 0;

  Tree* childImpl(StringRef Name) {
    return &*Children.try_emplace(Name, DetailAllocator).first;
  }

public:  
  Tree(BumpPtrAllocator *);
  Tree* child(StringLiteral Name) { return childImpl(Name); }
  Tree* detail(StringRef Name) {
    return DetailAllocator ? child(DetailAllocator->copy(Name)) : this;
  }
  void add(unsigned Size) { Self += Size; }
};
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88411/new/

https://reviews.llvm.org/D88411