[PATCH] D55488: Add utility for dumping a label with child nodes
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 11 11:17:31 PST 2018
aaron.ballman added inline comments.
================
Comment at: lib/AST/ASTDumper.cpp:89
void dumpDecl(const Decl *D);
- void dumpStmt(const Stmt *S);
+ void dumpStmt(const Stmt *S, const std::string &label = {});
----------------
steveire wrote:
> aaron.ballman wrote:
> > Label
> >
> > Rather than using `{}`, how about `""` (same behavior, but looks more idiomatic).
> >
> > Why `std::string` instead of `StringRef`? I expect this will be called mostly with string literals, which saves an allocation. The other labels are using `const char *`, which would be another reasonable option. Whatever we go with, it'd be nice to make the label types agree across the calls.
> The actual print in TextTreeStructure is deferred, so it can't be `const char*` or `StringRef`.
I think I've convinced myself there are not lifetime issues here, but it's subtle. In the case where the default argument is used, it creates a temporary object that is lifetime extended for the duration of the full expression including the call to `dumpStmt()`. The capture of `Label` in the lambda in `addChild()` captures the reference by copy and initializes the implicit field for the capture to the temporary value while its still within its lifetime. So I don't think this introduces UB.
However, if that lambda ever changes the capture list to use capture defaults (which we typically prefer when capturing multiple entities) and someone uses `&` or `this` as the capture default, they're going to have a very hard-to-discover bug on their hands. :-/
Another approach is to use `StringRef` in these calls, but within `addChild()` change the lambda to capture a local `std::string` by copy. e.g.,
```
template <typename Fn>
void addChild(StringRef Label, Fn doAddChild) {
...
std::string LabelStr = Label.str();
auto dumpWithIndent = [this, doAddChild, LabelStr](bool isLastChild) {
...
};
...
```
(And when we upgrade to C++14 we can drop the local and just use an init capture instead.)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55488/new/
https://reviews.llvm.org/D55488
More information about the cfe-commits
mailing list