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

[Paul Robinson via Phabricator via llvm-commits]

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?


================
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.


https://reviews.llvm.org/D34570