Page MenuHomePhabricator

Add utility for dumping a label with child nodes

Authored by steveire on Dec 9 2018, 5:29 AM.



Use it to add optional label nodes to Stmt dumps. This preserves
behavior of InitExprList dump:

CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15> 'U [3]'
CHECK-NEXT: |-array filler: InitListExpr {{.+}} <col:15> 'U' field Field {{.+}} 'i' 'int'
CHECK-NEXT: `-InitListExpr {{.+}} <col:14> 'U' field Field {{.+}} 'i' 'int'
CHECK-NEXT: `-IntegerLiteral {{.+}} <col:14> 'int' 1

Diff Detail

rC Clang

Event Timeline

steveire created this revision.Dec 9 2018, 5:29 AM
steveire updated this revision to Diff 177419.Dec 9 2018, 6:07 AM

Use std::string

aaron.ballman added inline comments.Dec 10 2018, 11:46 AM
115 ↗(On Diff #177419)

label -> Label
doAddChild -> DoAddChild

117 ↗(On Diff #177419)

While correct, this is a bit too clever -- can you switch to using a more idiomatic return statement?


Label, DoDumpChild



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.


label -> Label

steveire updated this revision to Diff 177594.Dec 10 2018, 1:43 PM
steveire marked an inline comment as done.

Update with new approach

steveire edited the summary of this revision. (Show Details)Dec 10 2018, 1:44 PM
steveire added a reviewer: rsmith.
steveire updated this revision to Diff 177595.Dec 10 2018, 1:50 PM

Clean up API a bit.

steveire added inline comments.Dec 10 2018, 1:51 PM

The actual print in TextTreeStructure is deferred, so it can't be const char* or StringRef.

aaron.ballman added inline comments.Dec 11 2018, 11:14 AM

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.)

steveire updated this revision to Diff 180754.Jan 8 2019, 3:30 PM

Replace std::string with StringRef

@rsmith -- do you have opinions on the new format for how we display the child node? I think this is a more clear approach, but a second opinion would be nice.


Why move this comment down a line? Might as well leave it where it was before (feel free to add a full stop to the end of the comment though).


doAddChild -> DoAddChild


Can you add a newline above this comment to give it some visual space?


doAddChild -> DoAddChild


deffered -> deferred


dumpWithIndent -> DumpWithIndent
isLastChild -> IsLastChild


Elide braces

aaron.ballman accepted this revision.Jan 10 2019, 7:17 AM

Aside from cosmetic nits, this LGTM.

This revision is now accepted and ready to land.Jan 10 2019, 7:17 AM
This revision was automatically updated to reflect the committed changes.