[llvm-commits] fix the MSVC warning in include/llvm-c/Core.h

[Paul Robinson]

On Mon, Jan 23, 2012 at 12:05 PM, Kostya Serebryany <kcc at google.com> wrote:
>
>
> On Mon, Jan 23, 2012 at 12:01 PM, Eli Friedman <eli.friedman at gmail.com>
> wrote:
>>
>> On Mon, Jan 23, 2012 at 11:02 AM, Kostya Serebryany <kcc at google.com>
>> wrote:
>> > My previous change in include/llvm-c/Core.h that introduced 64-bit
>> > Attributes (r148553) caused a warning
>> > while building with MSVC. http://llvm.org/bugs/show_bug.cgi?id=11828
>> > The following patch fixes the problem (use "static const uint64_t"
>> > instead
>> > of enum).
>> > Ok to commit?
>> >
>> > --kcc
>> >
>> > Index: include/llvm-c/Core.h
>> > ===================================================================
>> > --- include/llvm-c/Core.h  (revision 148708)
>> > +++ include/llvm-c/Core.h  (working copy)
>> > @@ -92,7 +92,7 @@
>> >  /** Used to get the users and usees of a Value. See the llvm::Use
>> > class. */
>> >  typedef struct LLVMOpaqueUse *LLVMUseRef;
>> >
>> > -typedef enum {
>> > +static const uint64_t
>> >      LLVMZExtAttribute       = 1<<0,
>> >      LLVMSExtAttribute       = 1<<1,
>> >      LLVMNoReturnAttribute   = 1<<2,
>> > @@ -119,8 +119,8 @@
>> >      LLVMReturnsTwice = 1 << 29,
>> >      LLVMUWTable = 1 << 30,
>> >      LLVMNonLazyBind = 1U << 31,
>> > -    LLVMAddressSafety = 1ULL << 32
>> > -} LLVMAttribute;
>> > +    LLVMAddressSafety = 1ULL << 32;
>> > +typedef uint64_t LLVMAttribute;
>> >
>> >  typedef enum {
>> >    /* Terminator Instructions */
>>
>> Hmm... actually, I'm not sure this is okay; it's a
>> non-binary-compatible change to the C API.
>
>
> Any other suggestion?
> It's not easy to keep compatibility once the new (beyond 32-bits) attributes
> start getting used.
> Maybe add "enum LLVMAttribute2" for attributes in bits 33-64?
>
> --kcc
>
>>
>>
>> -Eli
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

I've run into this kind of problem before.

A separate enum for attributes >= 32 is a royal pain.
It's an implementation artifact that has no relevance to the
semantics of the attributes.
You'd need to add a new API to take the upper attributes,
and the caller has to remember (or look up, every time)
which attributes go with which enum.  :-P

Defining a new 64-bit type that understands all attributes
is better, as all attributes can be handled the same way.
You still need a new API, but the caller does not have to
understand some arbitrary split between two different enums.

Pogo