Re: [006/nnn] poly_int: tree constants

[Richard Sandiford <richard.sandiford@linaro.org>]

Martin Sebor <msebor@gmail.com> writes:
>>>>>>  /* 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.

But that kind of situation is one where using "T (0)" over "T ()"
is useful.  It means that template substitution will succeed for
T that are sufficiently integer-like to have a single well-defined
zero but not for T that aren't (such as wide_int).

>>> 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 think it'd be surprising.  offset_int should always be used in
preference to wide_int if the precision is known to be 128 bits
in advance, and there doesn't seem any reason to prefer the
precision of offset_int over widest_int, HOST_WIDE_INT or int.

We would end up with:

  wide_int
  f (const wide_int &y)
  {
    wide_int x;
    x += y;
    return x;
  }

being valid if y happens to have 128 bits as well, and a runtime error
otherwise.

Also, I think it'd be inconsistent to allow the specific case of 0
to be assigned by default construction, but not also allow:

  wide_int x (0);

  wide_int x;
  x = 0;

  wide_int x;
  x = 1;

etc.  And wide_int wasn't intended for that use case.

Thanks,
Richard