<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Mar 26, 2014 at 2:02 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Wed, Mar 26, 2014 at 1:59 PM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Wed, Mar 26, 2014 at 1:55 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Wed, Mar 26, 2014 at 1:41 PM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> wrote:<br>
>> > Author: eliben<br>
>> > Date: Wed Mar 26 15:41:15 2014<br>
>> > New Revision: 204851<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=204851&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=204851&view=rev</a><br>
>> > Log:<br>
>> > Fix problem with r204836<br>
>> ><br>
>> > In CallInst, op_end() points at the callee, which we don't want to<br>
>> > iterate over<br>
>> > when just iterating over arguments. Now take this into account when<br>
>> > returning<br>
>> > a iterator_range from arg_operands. Similar reasoning for InvokeInst.<br>
>><br>
>> Having operands() differ from op_begin()/op_end() seems just a tad<br>
>> subtle...<br>
>><br>
>> Could we rename the existing range (op_begin/op_end) to something more<br>
>> distinct (or rename the new one to something more distinct) to<br>
>> separate these two concepts more clearly?<br>
><br>
><br>
> Note that the range adapter is arg_operands(), rather than operands().<br>
<br>
</div>Could it just be called "args" or "arguments"?<br></blockquote><div><br></div><div>I guess they could, but as I mentioned earlier right now there's a convenient parallel between arg_operands and getArgOperand, which are two ways to get to the same thing. Calling it "args" would make this parallel less clear: args() and getArgOperand() -- so why not getArg() then? I suppose that whichever paint we pick for this bikeshed, it should be consistent with the existing idioms. Don't you think?</div>
<div><br></div><div>Eli</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> This<br>
> distinction, while subtle, is important because the existing accessors are<br>
> also called getArgOperand. So there already is a distinction between<br>
> "operands" (generic operand traits defined by OperandTraits and including<br>
> *all* the operands, like the callee in CallInst, for example) and "arg<br>
> operands". The new adapters I added are called arg_operands() for this<br>
> reason, and not operands().<br>
><br>
> I don't think we can rename op_begin/op_end - these come through macros from<br>
> OperandTraits.h and too much within LLVM depends on them.<br>
<br>
</div>Oh, that makes sense - they're the operands of the user.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> Eli<br>
><br>
><br>
><br>
>><br>
>><br>
>> ><br>
>> > Also adds a unit test to verify this actually works as expected.<br>
>> ><br>
>> ><br>
>> > Modified:<br>
>> > llvm/trunk/include/llvm/IR/Instructions.h<br>
>> > llvm/trunk/unittests/IR/InstructionsTest.cpp<br>
>> ><br>
>> > Modified: llvm/trunk/include/llvm/IR/Instructions.h<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=204851&r1=204850&r2=204851&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=204851&r1=204850&r2=204851&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/include/llvm/IR/Instructions.h (original)<br>
>> > +++ llvm/trunk/include/llvm/IR/Instructions.h Wed Mar 26 15:41:15 2014<br>
>> > @@ -1297,12 +1297,14 @@ public:<br>
>> ><br>
>> > /// arg_operands - iteration adapter for range-for loops.<br>
>> > iterator_range<op_iterator> arg_operands() {<br>
>> > - return iterator_range<op_iterator>(op_begin(), op_end());<br>
>> > + // The last operand in the op list is the callee - it's not one of<br>
>> > the args<br>
>> > + // so we don't want to iterate over it.<br>
>> > + return iterator_range<op_iterator>(op_begin(), op_end() - 1);<br>
>> > }<br>
>> ><br>
>> > /// arg_operands - iteration adapter for range-for loops.<br>
>> > iterator_range<const_op_iterator> arg_operands() const {<br>
>> > - return iterator_range<const_op_iterator>(op_begin(), op_end());<br>
>> > + return iterator_range<const_op_iterator>(op_begin(), op_end() - 1);<br>
>> > }<br>
>> ><br>
>> > /// \brief Wrappers for getting the \c Use of a call argument.<br>
>> > @@ -2954,12 +2956,12 @@ public:<br>
>> ><br>
>> > /// arg_operands - iteration adapter for range-for loops.<br>
>> > iterator_range<op_iterator> arg_operands() {<br>
>> > - return iterator_range<op_iterator>(op_begin(), op_end());<br>
>> > + return iterator_range<op_iterator>(op_begin(), op_end() - 3);<br>
>> > }<br>
>> ><br>
>> > /// arg_operands - iteration adapter for range-for loops.<br>
>> > iterator_range<const_op_iterator> arg_operands() const {<br>
>> > - return iterator_range<const_op_iterator>(op_begin(), op_end());<br>
>> > + return iterator_range<const_op_iterator>(op_begin(), op_end() - 3);<br>
>> > }<br>
>> ><br>
>> > /// \brief Wrappers for getting the \c Use of a invoke argument.<br>
>> ><br>
>> > Modified: llvm/trunk/unittests/IR/InstructionsTest.cpp<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=204851&r1=204850&r2=204851&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=204851&r1=204850&r2=204851&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/unittests/IR/InstructionsTest.cpp (original)<br>
>> > +++ llvm/trunk/unittests/IR/InstructionsTest.cpp Wed Mar 26 15:41:15<br>
>> > 2014<br>
>> > @@ -14,11 +14,14 @@<br>
>> > #include "llvm/IR/Constants.h"<br>
>> > #include "llvm/IR/DataLayout.h"<br>
>> > #include "llvm/IR/DerivedTypes.h"<br>
>> > +#include "llvm/IR/Function.h"<br>
>> > #include "llvm/IR/IRBuilder.h"<br>
>> > #include "llvm/IR/LLVMContext.h"<br>
>> > #include "llvm/IR/MDBuilder.h"<br>
>> > +#include "llvm/IR/Module.h"<br>
>> > #include "llvm/IR/Operator.h"<br>
>> > #include "gtest/gtest.h"<br>
>> > +#include <memory><br>
>> ><br>
>> > namespace llvm {<br>
>> > namespace {<br>
>> > @@ -47,6 +50,29 @@ TEST(InstructionsTest, ReturnInst) {<br>
>> > delete r1;<br>
>> > }<br>
>> ><br>
>> > +TEST(InstructionsTest, CallInst) {<br>
>> > + LLVMContext &C(getGlobalContext());<br>
>> > + std::unique_ptr<Module> M(new Module("MyModule", C));<br>
>> > +<br>
>> > + Type *ArgTypes[] = {Type::getInt8Ty(C), Type::getInt32Ty(C),<br>
>> > + Type::getInt64Ty(C)};<br>
>> > + FunctionType *FTy = FunctionType::get(Type::getVoidTy(C), ArgTypes,<br>
>> > false);<br>
>> > + Function *F = Function::Create(FTy, Function::ExternalLinkage, "",<br>
>> > M.get());<br>
>> > +<br>
>> > + Value *Args[] = {ConstantInt::get(Type::getInt8Ty(C), 20),<br>
>> > + ConstantInt::get(Type::getInt32Ty(C), 9999),<br>
>> > + ConstantInt::get(Type::getInt64Ty(C), 42)};<br>
>> > + CallInst *Call = CallInst::Create(F, Args);<br>
>> > +<br>
>> > + // Make sure iteration over a call's arguments works as expected.<br>
>> > + unsigned Idx = 0;<br>
>> > + for (Value *Arg : Call->arg_operands()) {<br>
>> > + EXPECT_EQ(ArgTypes[Idx], Arg->getType());<br>
>> > + EXPECT_EQ(Call->getArgOperand(Idx)->getType(), Arg->getType());<br>
>> > + Idx++;<br>
>> > + }<br>
>> > +}<br>
>> > +<br>
>> > TEST(InstructionsTest, BranchInst) {<br>
>> > LLVMContext &C(getGlobalContext());<br>
>> ><br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>