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

[David Blaikie via llvm-commits]

On Fri, Jun 23, 2017 at 5:12 PM Robinson, Paul <paul.robinson at sony.com>
wrote:

> ================
> 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.
>
>
>
> So we would have something like:
>
>     Optional<uint8_t> getFixedByteSize(dwarf::Form Form, const
> DWARFFormParams &Params);
>
>     Optional<uint8_t> getFixedByteSizeForAbbrev(dwarf::Form Form);
>
> with commentary reinforcing the special case.  I can do that.
>

I was thinking abbrev call site could just call: getFixedByteSize(Form,
DWARFFormParams());

But sure, a separate function that does that might be good. Dunno.

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)


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