Page MenuHomePhabricator

[analyzer] NFC: Make sure we don't ever inline the constructor for which the temporary destructor is noreturn and missing.
ClosedPublic

Authored by NoQ on Jan 31 2018, 7:25 PM.

Details

Summary

Because we're planning to add more cases when temporary constructors will be inlined, it is important to document the fact that the current heuristic of terminating the analysis upon encountering constructors that correspond to noreturn temporary destructors does not work in this case.

For now these constructors are not inlined for the sole reason that their respective destructors are non-trivial and missing from the CFG. We will hopefully be able to enable temporary destructors in the CFG by default, so it won't be an issue. However, the intent is to maintain the mode without temporary destructors usable in short-term, and this assertion will force us to put the checks for presence of the destructor in the CFG wherever we enable inlining temporary constructors.

In other words, if any new code violates this assertion, it should be fixed by un-inlining this constructor when -analyzer-config cfg-temporary-dtors is set to false.

Diff Detail

Event Timeline

NoQ created this revision.Jan 31 2018, 7:25 PM
NoQ updated this revision to Diff 132316.Jan 31 2018, 7:37 PM

Add a comment explaining that it's not really bad to inline the constructor, but we simply have the sink not implemented in this case.

dcoughlin accepted this revision.Feb 7 2018, 3:47 PM

This looks good to me, but I think you should include your explanatory comments in the commit message to the comment itself to help future violators of the assertion out!

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
362

Can you add to the comment the instructions from the commit message on what to do if the assertion fires? I..e, "If any new code violates this assertion, it should be fixed by not inlining this constructor when -analyzer-config cfg-temporary-dtors is set to false."

This will give someone who violates the assertion concrete steps to take.

This revision is now accepted and ready to land.Feb 7 2018, 3:47 PM
NoQ updated this revision to Diff 133675.Feb 9 2018, 1:09 PM
NoQ marked an inline comment as done.

Add the comment.

This revision was automatically updated to reflect the committed changes.