sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:22
+/// preserving the hierarchy.
+struct MemoryTree {
+public:
----------------
(I'm wondering if there are other resources we'd like to count, like disk size
or so, but probably YAGNI and a concrete name is nice)
================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:24
+public:
+ void addChild(llvm::StringRef Name, MemoryTree MT, bool IsDetail = false);
+ void addChild(llvm::StringRef Name, size_t Size);
----------------
The concept of "detail" should be introduced somewhere, or just be something
more concrete
(like IsFilename)
There's a few things about filenames that make erasing them sensible:
- they have uniform semantics - we don't really know the difference between
them
- can be very many of them so the result is inherently hard to interpret
- can be many/large so we don't want to copy them
Thinking about how to generalize these - I think the name detail is OK (if you
want a general name) but we should mention some of it.
================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:25
+ void addChild(llvm::StringRef Name, MemoryTree MT, bool IsDetail = false);
+ void addChild(llvm::StringRef Name, size_t Size);
+
----------------
do we support nodes with both usage and children?
This API makes such a construction awkward.
vs something like addUsage(size_t)
================
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*/,
----------------
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);
}
```
================
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:
> 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?
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
