SSA range class and removal of VR_ANTI_RANGEs

[Aldy Hernandez]

On 06/20/2017 10:59 AM, Martin Sebor wrote:
> On 06/20/2017 02:41 AM, Aldy Hernandez wrote:
>>
>>
>> On 05/23/2017 03:26 PM, Martin Sebor wrote:
>>> On 05/23/2017 04:48 AM, Aldy Hernandez wrote:
>>
>>> +  void Union (wide_int x, wide_int y);
>>> +  bool Union (const irange &r);
>>> +  bool Union (const irange &r1, const irange &r2);
>>> +
>>> +  // THIS = THIS ^ [X,Y].  Return TRUE if result is non-empty.
>>> +  bool Intersect (wide_int x, wide_int y, bool readonly = false);
>>> +  // THIS = THIS ^ R.  Return TRUE if result is non-empty.
>>> +  // THIS = R1 ^ R2.  Return TRUE if result is non-empty.
>>> +  bool Intersect (const irange &r1, const irange &r2, bool readonly =
>>> false);
>>> +  // Return TRUE if THIS ^ R will be non-empty.
>>> +  bool Intersect_p (const irange &r)
>>> +    { return Intersect (r, /*readonly=*/true); }
>>>
>>> I would suggest the following changes to Union, Intersect, and Not:
>>>
>>> 1) Define all three members without the readonly argument and
>>>     returning irange& (i.e., *this).  The return value can be
>>>     used wherever irange& is expected, and the is_empty() member
>>>     function can be called on it to obtain the same result.  E.g.,
>>>     Intersect A with B, storing the result in A:
>>>
>>>     irange A, B;
>>>     if (A.Intersect (B).is_empty ()) { ... }
>>>
>>> 2) Add non-members like so:
>>>
>>>    irange range_union (const irange &lhs, const irange &rhs)
>>>    {
>>>      return irange (lhs).Union (rhs);
>>>    }
>>>
>>>    and find out if the union of A or B is empty without modifying
>>>    either argument:
>>>
>>>    irange A, B;
>>>    if (range_union (A, B).is_empty ()) { ... }
>>
>> Perhaps we could provide an implicit conversion from irange to bool such
>> that we could write:
>>
>> if (range_union (A, B)) { ... }
>>
>> as well as being able to write:
>>
>> if (!range_union (A, B).is_empty ()) { ... }
>>
>> That is, have range_union() return an irange as suggested, but have a
>> bool overload (or whatever the C++ nomenclature is) such that converting
>> an irange to a bool is interpreted as ``nitems != 0''.
>>
>> Is this acceptable C++ practice?
> 
> Implicit conversion to bool is a common way of testing validity
> but I don't think it would be too surprising to use it as a test
> for non-emptiness.  An alternative to consider is to provide
> an implicit conversion to an unsigned integer (instead of
> num_ranges()(*)) and have it return the number of ranges.  That
> will make it possible to do the same thing as above while also
> simplifying the API.

I really like this.

I have added an implicit conversion to unsigned that returns the number 
of sub-ranges.  However, I have also left the empty_p() method for a 
cleaner interface within the class itself.  It feels wrong to type "if 
(!*this)" to represent emptiness within the class.

> 
> Martin
> 
> [*] FWIW, there's nothing wrong with the name num_ranges() but
> those familiar with the C++ standard library are going to be
> accustomed to size() as the name of a function that returns
> the number of elements in a container.  Since the irange class
> is an ordered sequence of ranges, size() would work for it too.

In my upcoming patch I have renamed num_ranges() to num_pairs() which I 
thought was clearer, but if you still feel strongly about this, I can 
rename to size().

> 
> PS Thinking of the irange class as a container of ranges suggests
> the design might benefit from introducing a simple lower-level
> abstraction (class) for a single contiguous range.

Hmmm, indeed.  Perhaps when the dust settles with the patch I'm about to 
post :).

Thanks for your suggestions.
Aldy