This is the mail archive of the gcc-patches@gcc.gnu.org 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: [006/nnn] poly_int: tree constants


 /* The tree and const_tree overload templates.   */
 namespace wi
 {
+  class unextended_tree
+  {
+  private:
+    const_tree m_t;
+
+  public:
+    unextended_tree () {}

Defining no-op ctors is quite dangerous and error-prone.  I suggest
to instead default initialize the member(s):

   unextended_tree (): m_t () {}

Ditto everywhere else, such as in:

This is really performance-senesitive code though, so I don't think
we want to add any unnecessary initialisation.  Primitive types are
uninitalised by default too, and the point of this class is to
provide an integer-like interface.

I understand the performance concern (more on that below), but
to clarify the usability issues,  I don't think the analogy with
primitive types is quite fitting here: int() evaluates to zero,
as do the values of i and a[0] and a[1] after an object of type
S is constructed using its default ctor, i.e., S ():

   struct S {
     int i;
     int a[2];

     S (): i (), a () { }
   };

Sure, I realise that.  I meant that:

  int x;

doesn't initialise x to zero.  So it's a question of which case is the
most motivating one: using "x ()" to initialise x to 0 in a constructor
or "int x;" to declare a variable of type x, uninitialised.  I think the
latter use case is much more common (at least in GCC).  Rearranging
things, I said later:

I agree that the latter use case is more common in GCC, but I don't
see it as a good thing.  GCC was written in C and most code still
uses now outdated C practices such as declaring variables at the top
of a (often long) function, and usually without initializing them.
It's been established that it's far better to declare variables with
the smallest scope, and to initialize them on declaration.  Compilers
are smart enough these days to eliminate redundant initialization or
assignments.

In your other message you used the example of explicit default
initialisation, such as:

class foo
{
  foo () : x () {}
  unextended_tree x;
};

But I think we should strongly discourage that kind of thing.
If someone wants to initialise x to a particular value, like
integer_zero_node, then it would be better to do it explicitly.
If they don't care what the initial value is, then for these
integer-mimicing classes, uninitialised is as good as anything
else. :-)

What I meant was: if you want to initialise "i" to 1 in your example,
you'd have to write "i (1)".  Being able to write "i ()" instead of
"i (0)" saves one character but I don't think it adds much clarity.
Explicitly initialising something only seems worthwhile if you say
what you're initialising it to.

My comment is not motivated by convenience.  What I'm concerned
about is that defining a default ctor to be a no-op defeats the
zero-initialization semantics most users expect of T().

This is particularly concerning for a class designed to behave
like an [improved] basic integer type.  Such a class should act
as closely as possible to the type it emulates and in the least
surprising ways.  Any sort of a deviation that replaces well-
defined behavior with undefined is a gotcha and a bug waiting
to happen.

It's also a concern in generic (template) contexts where T() is
expected to zero-initialize.  A template designed to work with
a fundamental integer type should also work with a user-defined
type designed to behave like an integer.

With the new (and some existing) classes that's not so, and it
makes them harder and more error-prone to use (I just recently
learned this the hard way about offset_int and the debugging
experience is still fresh in my memory).

Sorry about the bad experience.  But that kind of thing cuts
both ways.  If I write:

poly_int64
foo (void)
{
  poly_int64 x;
  x += 2;
  return x;
}

then I get a warning about x being used uninitialised, without
having had to run anything.  If we add default initialisation
then this becomes something that has to be debugged against
a particular test case, i.e. we've stopped the compiler from
giving us useful static analysis.

With default initialization the code above becomes valid and has
the expected effect of adding 2 to zero.  It's just more robust
than the same code with that uses a basic type instead.  This
seems no more unexpected and no less desirable than the well-
defined semantics of something like:

  std::string x;
  x += "2";
  return x;

or using any other C++ standard library type in a similar way.

(Incidentally, although I haven't tried with poly_int, I get no
warnings for the code above with offset_int or wide_int.)

When the cor is inline and the initialization unnecessary then
GCC will in most instances eliminate it, so I also don't think
the suggested change would have a significant impact on
the efficiency of optimized code, but...

...if it is thought essential to provide a no-op ctor, I would
suggest to consider making its property explicit, e.g., like so:

   struct unextended_tree {

     struct Uninit { };

     // ...
     unextended_tree (Uninit) { /* no initialization */ }
     // ...
   };

This way the programmer has to explicitly opt in to using the
unsafe ctor.  (This ctor is suitable for single objects, not
arrays of such things, but presumably that would be sufficient.
If not, there are tricks to make that work too.)

The default constructors for unextended_tree and extended_tree
are only there for the array case (in poly-int.h).

My main concern is with the new poly_int classes (and the existing
wide int classes) because I think those are or will be widely used,
far more so than the unextended_tree class (I confess this review
is the first time I've ever noticed it).

Part of the problem here is that we still have to live by C++03
POD rules.  If we moved to C++11, the need for the poly_int_pod/
poly_int split would go away and things would probably be much
simpler. :-)

Understood.  With the heavy use of templates, template templates,
and partial specialization, the poly_int classes will push older
C++ 98 compilers to their limits.  It seems that for stability's
sake it would make sense to require a more modern compiler.

Note that for this class NULL_TREE is not a safe default value.
The same goes for the wide-int.h classes, where a length or precision
of 0 is undefined and isn't necessarily going to be handled gracefully
or predictably.

For offset_int both precision and length are known so I think
it would make sense to have the default ctor value-initialize
the object.  For wide_int, it seems to me that choosing some
default precision and length in the default ctor would still
be preferable to leaving the members indeterminate.  (That
functionality could still be provided by some other ctor as
I suggested above).

But which precision though?  If we pick a commonly-used one
then we make a missing initialisation bug very data-dependent.
Even if we pick a rarely-used one, we create a bug in which
the wide_int has the wrong precision even though all assignments
to it "obviously" have the right precision.

For offset_int the default precision is 128-bits.  Making that
the default also for wide_int should be unsurprising.

I only meant to suggest that the comment be reworded so as
not to imply that such pointers (that are farther apart than
PTRDIFF_MAX) can point to the same object and be subtracted.

OK, how about:

/* Return true if T holds a polynomial pointer difference, storing it in
   *VALUE if so.  A true return means that T's precision is no greater
   than 64 bits, which is the largest address space we support, so *VALUE
   never loses precision.  However, the signedness of the result does
   not necessarily match the signedness of T: sometimes an unsigned type
   like sizetype is used to encode a value that is actually negative.  */

That looks good to me.

Thanks
Martin


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