This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: SSA range class and removal of VR_ANTI_RANGEs

On 06/22/2017 06:16 AM, Richard Sandiford wrote:
Aldy Hernandez <> 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 =
+  // 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.


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]