On Oct 6, 2009, at 5:47 PM, Kenneth Uildriks wrote:> On Tue, Oct 6, 2009 at 2:13 PM, Kenneth Uildriks > <kennethuil at gmail.com> wrote: >> My front-end is sync'd with the trunk now, and working well, but it >> required some additional functions exposed in the C bindings. I >> hereby submit them for review and approval for inclusion in the >> trunk. >> > > LLVMGetAttribute had a bug in it. Here's the revised version of the > patchHi Kenneth, Thanks for working on this. I have some additional comments: +/** See the llvm::Use class. */ +typedef struct LLVMOpaqueUse *LLVMUseRef; My understanding is that this actually conceptually corresponds to use_iterator, not Use. Please name this something like LLVMUseIterator. Also, please document this, not just referring to llvm::Use. +int LLVMHasInitializer(LLVMValueRef GlobalVar); LLVMValueRef LLVMGetInitializer(LLVMValueRef GlobalVar); Isn't LLVMHasInitializer just LLVMGetInitializer(x) != 0? Otherwise, looks ok to me, -Chris
On Sun, Oct 11, 2009 at 3:09 PM, Chris Lattner <clattner at apple.com> wrote:> > On Oct 6, 2009, at 5:47 PM, Kenneth Uildriks wrote: > >> On Tue, Oct 6, 2009 at 2:13 PM, Kenneth Uildriks <kennethuil at gmail.com> >> wrote: >>> >>> My front-end is sync'd with the trunk now, and working well, but it >>> required some additional functions exposed in the C bindings. I >>> hereby submit them for review and approval for inclusion in the trunk. >>> >> >> LLVMGetAttribute had a bug in it. Here's the revised version of the patch > > Hi Kenneth, > > Thanks for working on this. I have some additional comments: > > > +/** See the llvm::Use class. */ > +typedef struct LLVMOpaqueUse *LLVMUseRef; > > My understanding is that this actually conceptually corresponds to > use_iterator, not Use. Please name this something like LLVMUseIterator. > Also, please document this, not just referring to llvm::Use.I was following the pattern of Functions, Globals, etc., where you get a Use* (not a use_iterator), and then pass it back to a GetNextUse call, which turns it back into an iterator and advances it.> > > +int LLVMHasInitializer(LLVMValueRef GlobalVar); > LLVMValueRef LLVMGetInitializer(LLVMValueRef GlobalVar); > > Isn't LLVMHasInitializer just LLVMGetInitializer(x) != 0? > > > Otherwise, looks ok to me, > > -Chris >So you want the whole patch, or just the pieces you highlighted?
On Sun, Oct 11, 2009 at 3:09 PM, Chris Lattner <clattner at apple.com> wrote:> > Isn't LLVMHasInitializer just LLVMGetInitializer(x) != 0?Last time I tried that, LLVMGetInitializer threw an assertion when the global variable didn't actually have one. Has this changed?
On Oct 11, 2009, at 1:25 PM, Kenneth Uildriks wrote:> On Sun, Oct 11, 2009 at 3:09 PM, Chris Lattner <clattner at apple.com> > wrote: >> >> Isn't LLVMHasInitializer just LLVMGetInitializer(x) != 0? > > Last time I tried that, LLVMGetInitializer threw an assertion when the > global variable didn't actually have one. Has this changed?No idea. It would be more C like to return null. The C implementation of the function can check and return null if not set.> I was following the pattern of Functions, Globals, etc., where you get > a Use* (not a use_iterator), and then pass it back to a GetNextUse > call, which turns it back into an iterator and advances it.Conceptually you're returning an iterator. It happens to be implemented as a tight wrapper around the Use.> > So you want the whole patch, or just the pieces you highlighted?Please resend an updated patch (the whole thing) -Chris