Re: SSA range class and removal of VR_ANTI_RANGEs

[Martin Sebor <msebor at gmail dot com>]

On 06/22/2017 06:16 AM, Richard Sandiford wrote:
> Aldy Hernandez <aldyh@redhat.com> writes:
> > 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.
>
> Conversions to bool and conversions to int both seem dangerous.
> E.g. people might try "range1 | range2" as a shorthand for taking the
> union of the ranges.  It'll compile but instead give the bitwise OR
> of the two range counts.

Yes, that's a valid point and the reason why conversion to "bool"
as a test for validity is sometimes implemented as a conversion
to some unique pointer type, e.g.,

  struct UniquePointerType;
  operator UniquePointerType* () const;

To avoid the same problem with an implicit conversion to integer
the bitwise operator overloads (and any others of concern) could
either be deleted from the overload set, e.g., using some sort
of an enable_if trick (or declared as private members), or defined
with the expected meaning (that could also obviate the problem with
the capitalization of Union() if the operators were the only way
to achieve that).

But I don't have a strong preference for any of these approaches.

Martin

> How about instead having helper functions like intersect_p,
> as for bitmaps?
>
> Thanks,
> Richard