Page MenuHomePhabricator

[CFG] [analyzer] Add C++17-specific variable and return value construction contexts.

Authored by NoQ on Mar 16 2018, 6:36 PM.



In C++17 copy elision is mandatory for variable and return value constructors (as long as it doesn't involve type conversion) which results in AST that does not contain elidable constructors in their usual places. In order to provide construction contexts in this scenario we need to cover more AST patterns.

This patch makes the CFG prepared for these scenarios by:

  • Adding two new construction context classes: CXX17ElidedCopyVariableConstructionContext and CXX17ElidedCopyReturnedValueConstructionContext. These are forks of SimpleVariableConstructionContext and ReturnedValueConstructionContext which contain the additional CXXBindTemporaryExpr when it's found in the AST. Additionally, ReturnedValueConstructionContext is renamed into SimpleReturnedValueConstructionContext. Finally, common abstract base classes are introduced: VariableConstructionContext and ReturnedValueConstructionContext.
  • Allowing CFGCXXRecordTypedCall element to accept VariableConstructionContext and ReturnedValueConstructionContext as its context.

CXXBindTemporaryExpr is assumed to be present iff the object has non-trivial destructor. If the object has trivial destructor then CXXBindTemporaryExpr is not there, and the AST is pretty much indistinguishable from the simple case so we re-use SimpleVariableConstructionContext and SimpleReturnedValueConstructionContext. I'm not entirely sure that this is indeed the same case, but for the purposes of the analyzer, which is the primary user of construction contexts, this seems fine because when the object has trivial destructor it is not scary to inline the constructor. When the object requires non-trivial destruction, the new construction context is provided, and the analyzer bails out (with the hope of falling back to the pre-temporary-support behavior) for now - but more work seems to be necessary on the analyzer side to prevent it from crashing during actual analysis.

Diff Detail


Event Timeline

NoQ created this revision.Mar 16 2018, 6:36 PM
NoQ added inline comments.Mar 16 2018, 6:43 PM
481–486 ↗(On Diff #138804)

I decided to include/document/test this part of CFG as well (even though previously it was omitted from the test) because while working on this patch i realized that it's completely correct. Previously i was ranting (@xazax.hun remembers me ranting) that i don't understand why do we need a second diamond in the CFG with two destructors on each path. But it's quite obvious: we destroy the original temporary at the end of the full-ex...-uhm-i-mean-statement, and then we need to destroy the lifetime-extended elidable copy of the respective temporary. So for the purposes of the CFG there are indeed two diamonds with correlated conditions - which will turn into two separate paths in the ExplodedGraph. It's nice to know that in C++17 we don't have that diamond at all - only the first diamond.

LGTM, subject to minor nits

172 ↗(On Diff #138804)


157 ↗(On Diff #138804)

👍 for explicit constructors

74 ↗(On Diff #138804)

else not needed, as previous statement returns

87 ↗(On Diff #138804)

this else also seems redundant

98 ↗(On Diff #138804)

...and this one as well

This revision is now accepted and ready to land.Mar 20 2018, 11:51 AM
NoQ updated this revision to Diff 139245.Mar 20 2018, 7:37 PM

Address review comments.

NoQ marked 2 inline comments as done.Mar 20 2018, 7:37 PM
dcoughlin accepted this revision.Mar 21 2018, 9:31 PM

LGTM. I really appreciate the extra documentation you've added, too.

This revision was automatically updated to reflect the committed changes.