[PATCH] D14363: [FunctionAttrs] Remove a loop, NFC refactor

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:324-325
@@ -323,15 +323,4 @@
 
-    bool Found = false;
-    Function::arg_iterator AI = F->arg_begin(), AE = F->arg_end();
-    for (CallSite::arg_iterator PI = CS.arg_begin(), PE = CS.arg_end();
-         PI != PE; ++PI, ++AI) {
-      if (AI == AE) {
-        assert(F->isVarArg() && "More params than args in non-varargs call");
-        Captured = true;
-        return true;
-      }
-      if (PI == U) {
-        Uses.push_back(&*AI);
-        Found = true;
-        break;
-      }
+    unsigned UseIndex =
+      std::distance(const_cast<const Use *>(CS->op_begin()), U);
+
----------------
chandlerc wrote:
> Why op_begin here rather than arg_begin?
> 
> Also, its a bit odd that the use index here is the correct increment amount for the function argument iterator -- where is the function operand to the callsite?
> 
> I'd at least leave some comments to remind the reader of the surprising indexing contract of argument operands vs. function operands and the function argument list.
> Why op_begin here rather than arg_begin?
> 
> Also, its a bit odd that the use index here is the correct increment
> amount for the function argument iterator -- where is the function
> operand to the callsite?

For callsites, `op_begin` == `arg_begin`.  The callee and the two
successor blocks (for invokes) are the last operands to the call or
invoke instruction.

I'll change this to use `arg_begin` instead, that will make things
more obvious.

> I'd at least leave some comments to remind the reader of the
> surprising indexing contract of argument operands vs. function
> operands and the function argument list.

Will do.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:334-336
@@ -341,1 +333,5 @@
+
+    auto FormalArgIt = F->arg_begin();
+    std::advance(FormalArgIt, UseIndex);
+    Uses.push_back(FormalArgIt);
     return false;
----------------
chandlerc wrote:
> This can be:
> 
>   Uses.push_back(std::next(F->arg_begin(), UseIndex));
I didn't know `std::next` took a parameter, thanks!  Will fix.


http://reviews.llvm.org/D14363