[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

Wed Sep 18 18:31:54 PDT 2019

dblaikie added a comment.

In D67647#1674781 <https://reviews.llvm.org/D67647#1674781>, @comex wrote:

> In D67647#1674773 <https://reviews.llvm.org/D67647#1674773>, @dblaikie wrote:
>
> > I wasn't expecting it to involve all this work - but instead to change "arguments()" to return ArrayRef. Though perhaps you didn't go that route to allow future fixes to the strict aliasing (by having an adapting iterator - or perhaps arguments() already uses such an iterator that doesn't hit the strict aliasing issue?) needing to do even more cleaunp work?
>
>
> Yeah, `arguments()` returns an adapting iterator.

Right - I was suggesting that could be changed. Would it be OK to you to change arguments() to return ArrayRef? Or would you rather avoid that to avoid baking in more dependence on that alias violation?

>>> - Add an `operator[]` implementation to `iterator_range`.
>> 
>> I'm not sure that's appropriate - not all types with begin() and end() (even those with random access iterators) would have op[], so it doesn't seem appropriate to add it/depend on it?
> 
> Random access iterators are supposed to have `operator[]`, according to:
>  http://www.cplusplus.com/reference/iterator/RandomAccessIterator/

Yep, I'm with you there

> But the use of a template ensures that it doesn't cause an error with types that don't have `operator[]`, whether they are marked as random-access or not.
> 
> I checked the spec for "real" C++20 ranges... it seems that random access ranges don't always have `operator[]`,

Yeah, that's where I've got different feelings - a general purpose utility to take any range (thing with begin/end) over random access iterators and do indexed lookup, I'd be fine with that (implemented as "r.begin()[n]") but adding op[] to the range itself then leads code to depend on that feature which isn't a general purpose range feature - so changing this iterator_range to some other range implementation might break that code - it'd be nice to keep iterator_range's interface minimal to allow implementation flexibility in the APIs that expose it.

Same reason I've pushed back on adding things like "empty()" or "size()", etc. Which are fine (& have been added in STLExtras) as free functions.

> but "view_interface" (which is a type of range) does:
>  https://eel.is/c++draft/view.interface

Fair - guess they've gone in a slightly different direction than we have in LLVM for now. I don't feel super strongly about it - but a bit.

> Anyway, I don't think it hurts to have it on this little imitation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647