[PATCH] D41695: [Metadata] Extend 'count' field of DISubrange to take a metadata node

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In https://reviews.llvm.org/D41695#983530, @sdesmalen wrote:

> In https://reviews.llvm.org/D41695#980475, @aprantl wrote:
>
> > we could also have a single
> >
> > `PointerUnion<ConstantInt,  DIlVariable> getCount()`
> >
> > method.
>
>
> So I tried this and after seeing this change it is not that much different from having `getConstantCountOrNull` or `getVariableCountOrNull`, or even having a `getRawCountNode()` and using a `dyn_cast<>` to get the results. Most accesses to `getCount` would look like:
>
>   if (auto *CI = SR->getCount().dyn_cast<ConstantInt*>()) ...
>
> The benefit of using PointerUnion is that we know the result of getCount() is either a ConstantInt, a DIVariable (or is invalid), but the same could be argued for `getConstantCountOrNull()` and `getVariableCountOrNull()`.


With two `...OrNull()` methods the interface suggests that both of them could be null, or both of them could be nonnull. With the PointerUnion it unambiguously encodes that this is an either or situation (though it still allows the both are null case, which we also don't need).

> The downside of using PointerUnion is that we can only support two kinds of count types, and not something like DIExpression or something else if that is ever needed in the future (although specifically for DIExpression we agreed this wasn't necessary for the purposes I had in mind, but I think you see my point).

Not really :-)
`PointerUnion.h` also defines a `PointerUnion3` and `PointerUnion4`.

> I would be tempted to stick with the current interface, but let me know if you want me to post the patch(es) that use PointerUnion.

Personally I think that the PointerUnion interface is nicer, but it's arguably not very important.

>> The scenario I was thinking of was with the previous format used for constant DIGlobalVariables. They used to have their constant value stored as a GlobalValue and when that GlobalValue was optimized away, we also lost it in the debug info. That situation however is different than what you are doing here, you are storing a ConstantInt directly, which I presume is never deleted by any IR transformation(?), so this should be safe.
> 
> I read this as 'don't need to make changes', is that correct?

Correct, thanks.


https://reviews.llvm.org/D41695