<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 23, 2017 at 5:12 PM Robinson, Paul <<a href="mailto:paul.robinson@sony.com">paul.robinson@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class="m_-7987570984397399677WordSection1">
<p class="MsoNormal">================<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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
</div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_-7987570984397399677WordSection1"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">So we would have something like:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">    Optional<uint8_t> getFixedByteSize(dwarf::Form Form, const DWARFFormParams &Params);<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">    Optional<uint8_t> getFixedByteSizeForAbbrev(dwarf::Form Form);<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">with commentary reinforcing the special case.  I can do that.</span></p></div></div></blockquote><div><br>I was thinking abbrev call site could just call: getFixedByteSize(Form, DWARFFormParams()); <br><br>But sure, a separate function that does that might be good. Dunno.<br><br>It'd be good if in some way this was checked somehow to ensure that the forms used by callers not giving DWARFFormParams don't need those properties. I was figuring some assertions at the callsite (but if it has a separate function, the assertions could go in that function)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class="m_-7987570984397399677WordSection1"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">--paulr<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-bottom:12.0pt"><a href="https://reviews.llvm.org/D34570" target="_blank">https://reviews.llvm.org/D34570</a><u></u><u></u></p>
</blockquote>
</div>
</div>
</div>
</div>
</div>

</blockquote></div></div>