[PATCH] D64925: [IR] Add getArg() method to Function class

[Henry Wildermuth via Phabricator via llvm-commits]

hmwildermuth added a comment.

In D64925#1604439 <https://reviews.llvm.org/D64925#1604439>, @tejohnson wrote:

> In D64925#1602949 <https://reviews.llvm.org/D64925#1602949>, @hmwildermuth wrote:
>
> > In D64925#1599789 <https://reviews.llvm.org/D64925#1599789>, @tejohnson wrote:
> >
> > > Suggest sending this change with patch that uses and tests it.
> >
> >
> > @tejohnson any more recommendations?
>
>
> It looks fine but is there a plan for a follow on patch that uses this new facility in the code (not just in a unit test)? Wondering since I see that the type of arg_begin() is actually "Argument *" so there is already a fair amount of code that just does something like either arg_begin()[index] or arg_begin()+index. Not opposed to adding a new interface for this, but wondering what the motivating case is.
>
> Also, for future patches - please upload patches with context (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface) to make them easier to review.


Thank you for your tip on the patch context, this is my first patch.

The idea is that the method name should be as descriptive as possible for the use; something like arg_begin()[index] is much less descriptive than getArg(index). The reason I originally wrote the patch is because I was writing an LLVM pass and needed a specific argument to a function, but the documentation was unclear to me at the time on how to do it and the use of arg_begin and the arg_iterator type added unnecessary confusion. There also seem to be similar implementations in other classes, e.g. getOperandList () and getOperandUse(index) in the User class, so there is somewhat of a precedent for this kind of facility.

In terms of using this facility, I have no distinct plan but I may go through and replace arg_begin()[index] as I come across them, and would recommend that others should as well to improve readability.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64925/new/

https://reviews.llvm.org/D64925