Page MenuHomePhabricator

Add utility for dumping a label with child nodes
ClosedPublic

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

Details

Summary

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

Repository
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
include/clang/AST/ASTDumperUtils.h
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?

lib/AST/ASTDumper.cpp
65

Label, DoDumpChild

86

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.

1691–1692

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
lib/AST/ASTDumper.cpp
86

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
lib/AST/ASTDumper.cpp
86

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.Tue, Jan 8, 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.

include/clang/AST/TextNodeDumper.h
43

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

45

doAddChild -> DoAddChild

48

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

51

doAddChild -> DoAddChild

66

deffered -> deferred

68

dumpWithIndent -> DumpWithIndent
isLastChild -> IsLastChild

84

Elide braces

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

Aside from cosmetic nits, this LGTM.

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