[PR c++/20103] failure to gimplify constructors for addressable types

[Alexandre Oliva]

On Mar  7, 2005, Mark Mitchell <mark@codesourcery.com> wrote:

> (a) we should never use "==" to compare types, because there's no
> guarantee that the compiler will continue to use "==" types in places
> where it does at present;

The guarantee is the code in get_target_expr.  That's also the only
case in which type equality preserving matters, and even then, only
when INITIAL is a CONSTRUCTOR.  For all other cases, if preserving
such equality mattered, we'd have had to handle those cases before at
the point I'm handling them now.

> Think about it this way: == has no semantic meaning in C++

Indeed, it's a front-end implementation detail.  It should be fine to
take advantage of it in the front-end.  It's not about the language.
The TARGET_EXPR itself is just working around a constraint imposed by
the gimplifier.

> So, using == means that you're doing something with types that doesn't
> have semantics grounded in the language, which doesn't make sense,
> except in places that are trying to get debugging information correct,
> etc.

So think of it this way: if we adopted COMPOUND_LITERAL_EXPR like
you're inclined to do, we'd have to do the very same kind of type
propagation after resolving the complete type of the initializer.
This is no different from what I'm proposing to do here.

And there's more: since there's no compiler-enforced equality AFAICT
between the type of the COMPOUND_LITERAL_EXPR and that of the VAR_DECL
enclosed in it, we'd have o make the propagation conditional in just
the same way.

> (b) you're applying a non-uniform transformation depending on
> non-local knowledge.  What I mean by "non-uniform" is that the
> assignment to the type of the TARGET_EXPR is conditional.

Conditional as in, if a condition held before, make sure it holds
after.  It really doesn't sound non-uniform to me.

Note that we never needed this condition to hold before; TARGET_EXPRs
just were not handled at all: we never emitted TARGET_EXPRs whose
types were arrays of unknown bounds, to be inferred from the
initializer length, before this change.

> If it were an unconditional assignment, I'd be less nervous;

Can't you just think of it as if we had, instead of TARGET_EXPR, a
TARGET_EXPR_WITH_INITIALIZER_TYPE and TARGET_EXPR_WITH_DIFFERENT_TYPE,
and the statement was unconditional for the former, and not present
for the latter?  This is a precise description of the properties at
hand.

Now what if, instead of using up two separate tree nodes, we use a
single tree node for them, and use a comparison between the type of
the initializer and the type of the target_expr to tell which case we
have?  Hey!, but this is *exactly* what the patch does!

> then, you'd be asserting that the type of the TARGET_EXPR should
> always be the type of its initializer, which might make sense.

Further investigation has shown that TARGET_EXPR's SLOTs always have
the same type as the enclosing TARGET_EXPR.  Whether SLOTs and
INITIALs always have the same type is not as obvious, but it appears
to hold as well.  How about making the assignments unconditional,
then, with an assertion that the equality holds?

> What I mean by "non-local" is that your transformation only works
> because of some far-off logic that determins how TARGET_EXPRs are
> created.

No, because of some local preservation of a property.  Not preserving
it when it was present does break code.  Introducing it when it wasn't
there before might break code.  So the right thing to do is,
obviously, to test for the property, and preserve it if it was
present.  There isn't any non-local property here: the decision is
entirely local, and there's no hidden dependency on anything
elsewhere.  It's as simple as: if both entities pointed to the same
thing, make sure they still do afterwards.  How can this possibly not
be a reasonable thing to do?

> It doesn't depend on any documented property of TARGET_EXPRs that is
> enforced throughout the compiler.

It's precisely because the property is not documented that it's tested
locally.  Perhaps the property is guaranteed by the current
implementation, I can't quite tell for sure.  But testing for the
property and propagating it into the substed template feels like the
very right thing to do for some property that isn't necessarily
guaranteed.

> Consider the comments you should write for your code:

How about:

/* If the type of the initializer was used to create the original
   TARGET_EXPR, make sure we adjust the type of the tsubsted
   TARGET_EXPR, should the type of the initializer change in
   unpredictable ways during tsubsting (e.g., the range of an array is
   inferred from a CONSTRUCTOR length).  */

See?  No need to change any other piece of code anywhere else.  It's
really that simple.

> I'm sorry you're frustruated, but I don't think that your approach is
> the right way to go.

>>> But, minor changes elsewhere might make them same_type_p, but not
>>> ==, in some cases.
>> If that's fine for those cases, it will remain so after template
>> substitution.  Sounds *exactly* like what I want.

> Why would you want that?  If before substitution, the operand had a
> typedef type for the incomplete array,

Err, no.  Before substitution, the TARGET_EXPR was created using the
type obtained from the initializer.  That's the only case we care
about preserving.  Other cases weren't preserved before, so I don't
care about preserving them.

> and the TARGET_EXPR had a different typedef type for the same
> incomplete array, wouldn't you want to update the type of the
> TARGET_EXPR?

No, that's the point you're missing.  TARGET_EXPRs are not a C++
language concept.  They're an implementation detail of the compiler.
We even have COMPOUND_LITERAL_EXPR in the tree docs, but I don't see
that they're going to get destructors called at the right time, and
we'd still have to maintain the same equality, probably in the very
same conditional way, except that it's trickier because the
CONSTRUCTOR will be hiding deeper down in the tree.

> In the context of a 4.0 patch, I'd be willing to consider a patch like
> yours, using same_type_p instead of "==", and with suitable comments
> explaining what's going on.  For 4.1, we should do better.

same_type_p doesn't make sense for this purpose precisely because it's
a language concept, but what I'm dealing with here is an
implementation concept.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}