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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
