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

[Kostya Serebryany]

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


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120123/40132b93/attachment.html>