[PATCH] D88411: [clangd] Introduce MemoryTrees

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

LGTM, thanks!



================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:57
+  /// Returns total number of bytes used by this sub-tree. Performs a traversal.
+  size_t total() const;
+
----------------
oops, we should probably also expose self?


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return &createChild(Name); }
+
----------------
kadircet wrote:
> sammccall wrote:
> > sammccall wrote:
> > > actually, why do these return pointers rather than references?
> > reading call sites, `child()` might be both more fluent and more accurate than `addChild` - we're not calling it for the side effect and there may or may not be one.
> > actually, why do these return pointers rather than references?
> 
> It is to make usage with `auto` safer, as it won't capture ref by default and make a copy. e.g. `auto child = MT.child(); child.addusage(1)` won't work as expected.
> 
> I suppose we can make the object non-copyable and prevent such misuse.
Oh, right - yes ref but noncopyable seems even better to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411