[PATCH] D26594: IR: Change the gep_type_iterator API to avoid always exposing the "current" type.

[Peter Collingbourne via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D26594#602867, @dblaikie wrote:

> Seems odd to have an iterator-like thing without any op*, though.
>
> Should we make op* return 'getIndexedType' (I realize this is a subtle API break to change the semantics of an existing operation - perhaps better to leave that as a cleanup a version or two from now (leave a FIXME)?)


Yes, it was the semantics break I was concerned about. Good idea to add the FIXME, will do.

> I think you've stared at this code a fair bit more than I have (certainly more than I have recently - I'm probably the one to blame for the AddressSpace parameter being plumbed through a year ago or something) - could you summarize/explain why the need for such a variety of member functions on the iterator now? (getArray, isArray, isBoundedArray, getArrayBound, isStruct, getStruct, getStructOrNull)
> 
> It'd be nice if there were some more unifying API - currently this diversity seems a bit awkward.

Basically these are convenience functions around a small set of internal states (struct, unbounded array, bounded array) to help with readability at call sites.

In an earlier version of the patch [1] I had just two APIs: getStructType (which would return null for arrays) and getArrayBound (which could return a sentinel value meaning "unbounded"). Using those two functions, you can figure out which state you are in and effectively get the rest of the API, but that came at the cost of some readability (e.g. you would have to remember that "!getStructType" means "this is an array"). On balance it seemed better to have more API in gep_type_iterator.

Relatedly, I found that we only care about the array "boundedness" in relatively few places, which suggests to me that maybe we should look more closely at whether it is really necessary to expose that property and see if we can remove some of the API surface here. However, that seems like an orthogonal change to me.

[1] https://github.com/llvm-project/llvm-project/compare/master...pcc:pointertype


https://reviews.llvm.org/D26594