[PATCH] D34570: [DWARF] NFC: Collect info needed by DWARFFormValue into a helper

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 23 14:40:51 PDT 2017


On Fri, Jun 23, 2017 at 2:34 PM Paul Robinson via Phabricator <
reviews at reviews.llvm.org> wrote:

> probinson added a subscriber: clayborg.
> probinson added inline comments.
>
>
> ================
> Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h:29-63
> +class DWARFFormParams {
> +  uint16_t Version = 0;
> +  uint8_t AddrSize = 0;
> +  dwarf::DwarfFormat Format = dwarf::DWARF32;
> +
> +  // DWARFUnit and DWARFDebugLine use this as a data member initialized
> while
> +  // parsing the unit or line table, respectively.
> ----------------
> dblaikie wrote:
> > This seems like a bit more encapsulation than I'd expect/seems warranted
> for a few parameters.
> >
> > Especially the protected ctor, imho - it doesn't change any invariants
> of the class (the 3-arg ctor can be used to construct a default constructed
> object) & doesn't seem to protect from any really trivial misuse.
> >
> > Also the 3-arg ctor would be about as good as braced init, and the value
> can be changed at any time with assignment anyway - maybe make this a
> struct? & leave the getRefAddrByteSize+getDwarfOffsetByteSize member
> functions for convenience - but I'd strip the rest of the
> accessors/ctors/friending out and make it a struct.
> Yeah, I waffled on that.  I'll simplify.
> So to be clear, you're suggesting eliminating the 3-arg ctor as well?
>

Yeah, just make it a struct (drop the ctors, access modifiers, and
accessors) - leave the two helper functions that have non-trivial logic.

I can see the waffling - I don't wholely object to the way it is if you
particularly prefer it.


>
>
> ================
> Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:64
> +DWARFFormValue::getFixedByteSize(dwarf::Form Form,
> +                                 const DWARFFormParams *Params) {
>    switch (Form) {
> ----------------
> dblaikie wrote:
> > Should this be optional? The only place that doesn't pass it looks to be
> DWARFAbbreviationDeclaration::extract - is that usage correct? Does the
> abbrev table implicit_const not support the extra features? Should there be
> some error path/check for any forms that would require the params?
> There is a use-case for scanning the abbreviations table to determine
> which ones are fixed-size in all cases.  And the abbreviations table can be
> shared by units/line-tables with different formats/versions.  This was an
> important point for @clayborg and LLDB, I think.
> So yes, I think it should remain optional.  We can change our minds later
> if I'm wrong.
>

Still might be worth moving the optionality to that one use case & adding
some comments about why it's probably valid there so it doesn't get
accidentally propagated to other users.


>
>
> https://reviews.llvm.org/D34570
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170623/2a586de6/attachment.html>


More information about the llvm-commits mailing list