<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 23, 2017 at 2:34 PM Paul Robinson via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">probinson added a subscriber: clayborg.<br>
probinson added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h:29-63<br>
+class DWARFFormParams {<br>
+  uint16_t Version = 0;<br>
+  uint8_t AddrSize = 0;<br>
+  dwarf::DwarfFormat Format = dwarf::DWARF32;<br>
+<br>
+  // DWARFUnit and DWARFDebugLine use this as a data member initialized while<br>
+  // parsing the unit or line table, respectively.<br>
----------------<br>
dblaikie wrote:<br>
> This seems like a bit more encapsulation than I'd expect/seems warranted for a few parameters.<br>
><br>
> 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.<br>
><br>
> 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.<br>
Yeah, I waffled on that.  I'll simplify.<br>
So to be clear, you're suggesting eliminating the 3-arg ctor as well?<br></blockquote><div><br>Yeah, just make it a struct (drop the ctors, access modifiers, and accessors) - leave the two helper functions that have non-trivial logic.<br><br>I can see the waffling - I don't wholely object to the way it is if you particularly prefer it.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:64<br>
+DWARFFormValue::getFixedByteSize(dwarf::Form Form,<br>
+                                 const DWARFFormParams *Params) {<br>
   switch (Form) {<br>
----------------<br>
dblaikie wrote:<br>
> 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?<br>
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.<br>
So yes, I think it should remain optional.  We can change our minds later if I'm wrong.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D34570" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34570</a><br>
<br>
<br>
<br>
</blockquote></div></div>