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

[David Blaikie]

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

Given the constraints of the stable C API (binary compatibility)
there's no way we can make this forwards compatible, is there?
(passing a struct we could add elements to in the future - that would
break binary compat, 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
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits