[llvm-commits] [PATCH] C Bindings: Allow reading MDNode operands

[Villmow, Micah]

+ * The passed LLVMValueRef pointer should refer to an array of LLVMValueRef at
+ * least LLVMGetMDNodeNumOperands long.

I think a better wording here would be something like:
" The passed LLVMValueRef pointer should point to memory that contains enough space
to hold the number of LLVMValueRef's returned by LLVMGetMDNodeNumOperands."

+void LLVMGetMDNodeOperands(LLVMValueRef V, LLVMValueRef *Dest)
+{
+  const MDNode *N = cast<MDNode>(unwrap(V));
+  const unsigned numOperands = N->getNumOperands();
+  for (unsigned i = 0; i < numOperands; i++) {
+    Dest[i] = wrap(N->getOperand(i));
+  }
+}

What happens in the case where the LLVMValueRef is not an MDNode?

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Anthony Bryant
> Sent: Tuesday, September 18, 2012 8:36 PM
> To: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] C Bindings: Allow reading MDNode
> operands
> 
> Ping. Could someone have a look at this please? If there's nothing
> wrong with it, it would be good to get it committed.
> 
> On 13 September 2012 15:09, Anthony Bryant <antjbryant at gmail.com>
> wrote:
> > Hi Duncan,
> >
> >>>>> (Sorry for sending this to the wrong list earlier.)
> >>>>>
> >>>>> This patch just adds two functions: LLVMGetMDNodeNumOperands and
> >>>>> LLVMGetMDNodeOperands, which allow access to MDNode operands from
> >>>>> the C API, similarly to how named metadata can be accessed there.
> >>>>>
> >>>>> Could this be reviewed and possibly committed please?
> >>>>
> >>>> if the node isn't an MDNode shouldn't it barf (eg abort)?
> >>>
> >>> I thought I checked for that - both of them do dyn_cast<MDNode> and
> >>> check whether the result is null before continuing.
> >>>
> >>> Do you mean they should return some sort of error? Because the
> >>> functions that read the same things from NamedMDNodes don't.
> >>
> >> yes, that they should cause some sort of error.  I think routines
> >> like LLVMGetNamedMetadataNumOperands are wrong to work the way they
> >> do.  There should be a routine to get the named metadata pointer
> >> given the name (or null if there is no name).  There should be a
> >> routine to get or create named metadata given the name (and it
> should return a metadata pointer).
> >> Routines like LLVMGetNamedMetadataNumOperands should take the
> >> metadata pointer and ensure a crash if it isn't the right kind of
> >> pointer (eg by using cast rather than dyn_cast).
> >
> > Here's an updated patch that uses cast<MDNode> instead.
> > I would have made your suggested changes to the named metadata
> > functions too, but 1) The old functions would have to stay to be
> > backwards compatible, and they're using the names we would probably
> > want to use for the new functions, and 2) NamedMDNode isn't a Value,
> > so I'm not sure how to go about exposing it.
> >
> > Regardless, is this patch ok to go in now?
> >
> > Thanks,
> > Anthony.