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

[Paul Robinson]

On Mon, Jan 23, 2012 at 1:45 PM, Kostya Serebryany <kcc at google.com> wrote:
> There are 8 functions dealing with LLVMAttribute in include/llvm-c/Core.h.
> Do you suggest to add 8 more functions that will deal with uint64_t?
> Like this?
>
> void LLVMAddFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA);
> void LLVMAddFunctionAttr64(LLVMValueRef Fn, uint64_t PA);
>
> --kcc

With a more meaningful type name, e.g. LLVMAttribute64 or some such, yes;
I prefer that to 8 more functions that take pairs of 32-bit enum values.

void LLVMAddFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA);
void LLVMAddFunctionAttr2(LLVMValueRef Fn, LLVMAttribute PA,
LLVMAttribute2 PA2);

Or to 8 more functions that take the second set of enum values.

void LLVMAddFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA);
void LLVMAddFunctionAttr2(LLVMValueRef Fn, LLVMAttribute2 PA);

In all cases, the first function of each pair must be preserved for
compatibility;
the question is merely, what does the second function look like, and does the
caller have to care which enum a given attribute belongs to.
With the approach I suggested, I can convert to the new type and function
without regard to the implementation detail of the bit number being used for
the attribute I care about at the moment.  All I need to know is its name,
which is as it should be.

I can't see any way to both expand the attribute set, and preserve binary
compatibility, without defining 8 new functions of SOME kind.  In that case,
I would rather have a full new set of 8, and deprecate the old set of 8,
than have to contend with 16 functions all actively in use.

Pogo

>
> On Mon, Jan 23, 2012 at 1:40 PM, Paul Robinson <pogo.work at gmail.com> wrote:
>>
>> 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
>
>