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