This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Simplify DWARFAttribute. NFC.
ClosedPublic

Authored by ikudrin on Jul 9 2019, 5:46 AM.

Details

Summary

The first argument in the constructor was ignored, and the remaining arguments were always passed as their defaults.
This patch simplifies the class by moving all initializations to the members.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin created this revision.Jul 9 2019, 5:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 5:46 AM
Eugene.Zelenko added inline comments.Jul 9 2019, 6:12 AM
lib/DebugInfo/DWARF/DWARFDie.cpp
666 ↗(On Diff #208651)

You don't need to call default member constructor explicitly. Default member initialization could be used for Index too.

ikudrin updated this revision to Diff 208673.Jul 9 2019, 7:03 AM
ikudrin marked an inline comment as done.
  • Removed AttrValue from the member initializer list of the constructor of DWARFDie::attribute_iterator.
lib/DebugInfo/DWARF/DWARFDie.cpp
666 ↗(On Diff #208651)

The main intent here was to remove the first unused argument in the constructor of DWARFAttribute because it is a bit of misleading. And other changes in that class were done because there was no reason not to do them when I was there.
As for Index, I believe it is out of the scope of this patch because it is a member of a different class.

probinson added inline comments.
include/llvm/DebugInfo/DWARF/DWARFAttribute.h
32 ↗(On Diff #208673)

Form(0) is what the DWARFFormValue default ctor uses, you do not need to make this explicit here.

dblaikie added inline comments.Jul 9 2019, 4:54 PM
include/llvm/DebugInfo/DWARF/DWARFAttribute.h
34 ↗(On Diff #208673)

Remove this ctor since it's the same as the implicit default ctor now.

47–53 ↗(On Diff #208673)

Could you remove this function, and replace the one use (in DWARFDie.cpp) with "AttrValue = {};" instead, now that default construction has the desired effect.

ikudrin updated this revision to Diff 208904.Jul 10 2019, 2:43 AM
ikudrin added a reviewer: probinson.
  • Adjusted the initializer for Value.
  • Removed the constructor.
  • Removed the method clear() and replaced the only usage with AttrValue = {}.
dblaikie added inline comments.Jul 10 2019, 10:40 AM
include/llvm/DebugInfo/DWARF/DWARFAttribute.h
34 ↗(On Diff #208673)

This should be written as:

DWARFFormValue Value;

There's no need to explicitly default construct & then copy construct the object - the default constructor will be called implicitly.

I agree with all comments. After those few things are fixed, this looks good.

ikudrin updated this revision to Diff 209116.Jul 10 2019, 8:16 PM
ikudrin retitled this revision from [DWARF] Simplify constructing of DWARFAttribute. NFC. to [DWARF] Simplify DWARFAttribute. NFC..
  • Removed explicit initialization of Value.
ikudrin marked an inline comment as done.Jul 10 2019, 8:19 PM
ikudrin added inline comments.
include/llvm/DebugInfo/DWARF/DWARFAttribute.h
34 ↗(On Diff #208673)

Well, I usually prefer more explicit code because it is just simpler to understand, hoping that the optimizer will remove any redundancy. But let it be as you said.

dblaikie accepted this revision.Jul 15 2019, 9:33 PM
This revision is now accepted and ready to land.Jul 15 2019, 9:33 PM
This revision was automatically updated to reflect the committed changes.