kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+ /// No copy of the \p Name.
+ MemoryTree *addChild(llvm::StringLiteral Name) { return &createChild(Name); }
+
----------------
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.
================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:41
+ MemoryTree *addDetail(llvm::StringRef Name) {
+ return Alloc ? &createChild(llvm::StringSaver(*Alloc).save(Name)) : this;
+ }
----------------
sammccall wrote:
> nit: `Name.copy(*Alloc)` is a little more direct
ah thanks, didn't know that!
================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:31
+ /// detail to root, and doesn't report any descendants.
+ void traverseTree(bool CollapseDetails,
+ llvm::function_ref<void(size_t /*Size*/,
----------------
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > In earlier discussions we talked about avoiding the cost of
> > > > *assembling* the detailed tree, not just summarizing it at output time.
> > > >
> > > > This requires `CollapseDetails` to be passed into the profiling
> > > > function, which in turn suggests making it part of a tree and passing a
> > > > reference to the tree in. e.g. an API like
> > > >
> > > > ```
> > > > class MemoryTree {
> > > > MemoryTree(bool Detailed);
> > > > MemoryTree* addChild(StringLiteral); // no copy
> > > > MemoryTree* addDetail(StringRef); // returns `this` unless detailed
> > > > void addUsage(size_t);
> > > > }
> > > > ```
> > > It's hard to evaluate a tree-traversal API without its usages... how will
> > > this be used?
> > you can find some in https://reviews.llvm.org/D88414
> I guess you mean D88417?
>
> This works well when exporting all nodes somewhere keyed by dotted path, but
> it's pretty awkward+inefficient if you want to - totally hypothetical example
> here - dump the memory tree as a JSON structure over RPC.
>
> So I think we need a more flexible API, at which point... maybe we should
> punt, expose `const DenseMap<StringRef, MemoryTree> &children() const`, and
> move the traverse implementation to the metrics exporter?
SGTM.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88411/new/
https://reviews.llvm.org/D88411
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
