------- Additional Comments From aoliva at gcc dot gnu dot org 2005-03-08 07:24 ------- Subject: Re: [PR c++/20103] failure to gimplify constructors for addressable types
On Mar 7, 2005, Mark Mitchell <[EMAIL PROTECTED]> 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20103
