[llvm-commits] [PATCH] PR12696: Encode 'nonlazybind' and 'address_safety' properly in BitcodeWriter

[Meador Inge]

Ping.

On Fri, May 4, 2012 at 8:13 PM, Meador Inge <meadori at gmail.com> wrote:
> On Fri, May 4, 2012 at 12:25 PM, Chris Lattner <clattner at apple.com> wrote:
>
>> On May 3, 2012, at 10:26 PM, Meador Inge wrote:
>>
>>> Hi All,
>>>
>>> This patch fixes PR12696 which reported that the BitcodeWriter was not
>>> encoding the
>>> 'nonlazybind' and 'address_safety' attributes correctly.  Alessandro
>>> Pignotti suggested
>>> the original patch.  I wrote the test case and regression tested the change.
>>
>> This looks very close, but I don't like the magic numbers in the bitcode writer.  If there is no convenient enum in Attributes.h, can you add a new enum
>> with the magic number there?
>
> Which magic numbers, though?  In this particular case there are many
> magic numbers used for shifting and masking.  The complication arises
> from the fact that we are picking apart the attribute set to expand
> the alignment and then encoding that expanded alignment with the other
> attributes (a similar problem for decoding exist in the bitcode
> reader).  If the magic numbers were to be replaced for this case, then
> you would probably want enum/constants for all the masks and shifts.
> I think that is overkill.  Also, you can only go so for hiding the
> magic for bit twiddling stuff like this.
>
> One alternative is to hoist the operations out of the bitcode
> implementation into encoding/decoding inline functions in
> Attributes.h.  This way the bit twiddling is consolidated into a
> single place as a part of the attributes implementation (which we
> already do for similar operations like constructing alignments).  The
> attached patch implements this approach and extends the test case to
> cover all function and parameter attributes.
>
> Also, maybe addressing the "// FIXME: remove in LLVM 3.0" FIXMEs would
> simplify things.  Although, I am not completely sure what needs to be
> done to address those FIXMEs.  If (when) they are addressed at least
> with the inline function approach the places being fixed will be right
> next to each other :-)
>
> -- Meador



-- 
# Meador