[PATCH] D55488: Add utility for dumping a label with child nodes

[Aaron Ballman via Phabricator via cfe-commits]

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