Page MenuHomePhabricator

[DWARFExpression] Remove ctor that takes just a compile unit.

Authored by JDevlieghere on May 24 2019, 1:58 PM.



Like many of our DWARF classes, the DWARFExpression can be initialized in many ways. One such way was through a constructor that takes just the compile unit. This constructor is used to initialize both empty DWARFExpressions, and DWARFExpression that will be populated later. To make the distinction more clear, I changed the constructor to a default constructor and updated its call sites. Where the DWARFExpression was being populated later, I replaced that with a call to the copy assignment constructor.

Diff Detail

Event Timeline

JDevlieghere created this revision.May 24 2019, 1:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 1:58 PM


labath added inline comments.May 27 2019, 12:12 AM

This is not the copy assignment constructor. That would be something like:
*frame_base = DWARFExpression(...).
This code looks pretty dodgy, as you're trampling over an already constructed object. I'm not sure whether that in itself is UB, but it definitely is not a good idea.

labath added a comment.EditedMay 27 2019, 12:13 AM

(Other than that, the change seems like a nice improvement.)

clayborg added inline comments.May 28 2019, 8:05 AM

correct the comment or remove changes?


Use move?:

*frame_base = std::move(DWARFExpression(...));

use move like suggested above?

labath added inline comments.May 28 2019, 8:09 AM

move is not needed as DWARFExpression(...) is a temporary.

Address CR feedback

lgtm. Pavel, you good?

labath accepted this revision.May 28 2019, 9:43 AM
This revision is now accepted and ready to land.May 28 2019, 9:43 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 10:31 AM