tomasz-kaminski-sonarsource wrote:
> What I don't want to lose from this patch are the changes to places like
> `InitializationStyle getInitializationStyle() const` and
> `CXXNewExpr::CXXNewExpr` where the old code was unclear and the new code is
> significantly more clear. We should not be performing math on the enumerator
> values to encode in-band extra information. What I see being clarified by
> this patch is:
>
> ```
> struct S { int x; };
> auto *s0 = new int; // None, scalar types have no notional constructor
> initialization
> auto *s1 = new S; // Implicit, class type has a notional constructor call
> auto *s2 = new S(0); // Call (ParenList is a much better name)
> auto *s3 = new S{0}; // List (BraceList is a much better name)
> ```
>
> > In both cases, the news would report the initialization as Implicit, where
> > actually no initialization is performed. There is no call to the
> > constructor inserted.
>
> There is an implicit constructor call but it's a noop because the type is
> trivial, so I think `Implicit` is what I would expect given the comment `///
> New-expression has no written initializer, but has an implicit one.`
> https://godbolt.org/z/353G45vnc
>
> That said, I can see why it may be confusing to say there's an implicit
> initialization for something that is a noop which performs no initialization
> and we have an enumerator for "no initialization". I think
> @tomasz-kaminski-sonarsource would like for the extra enumerator to be
> removed, but I don't think that's possible to do without also losing the
> benefits of the changes. But perhaps we could rename `Implicit` and `None` to
> something more clear?
Yes, this is the thing that I am aiming for.
As far as changes LLVM code goes, I see that everywhere expect
`hasInitializer()` where `NoInit` is used, then `Implicit` is handled in the
same manner, so as far as this particular PR goes removing the `Implicit` will
make the code simpler. I do not think that changing public-facing API to
eliminate storage-hack is the right direction.
>
> > In short, as the downstream that uses AST for writing rules, we will need
> > to update all the uses of NoInit to also check for Implicit, without
> > getting any value from the distinction.
>
> The C++ APIs have no stability guarantees and not every change will be to the
> benefit of all downstreams; I see the changes in this PR as being an
> improvement over the status quo because they clarify code in our code base
> and I'm not seeing the same level of confusion you are in your downstream.
> (That said, I'm also totally happy to rename enumeration members to pick more
> descriptive names.)
With this change, I am no longer able to explain what the meaning of this
enumeration is supposed to be. Previously it was all about the syntax, and the
meaning of values was clear. Now we separate two cases, where we differentiate
lack of initializer being spelled between doing no constructor calls and doing
possibly implicit constructor calls.
If we go with that direction, we should also differentiate between parents
(`Call`) doing various semantics effects, such as calling constructor,
construction aggregate, or initializing built-in types. Same for the braces.
This would lead to an unwieldy number of the enumerator's values, which
illustrates the problem of trying to shoehorn two orthogonal aspects into one
enumerator.
For example, if we decide to introduce AST that would represent performing
default initialization of trivial object (for example to capture Erroneous
Behavior), all uses of `NoInit` would turn into `Implicit` because there was an
initializer node.
I am less concerned about the number of changes required downstream, but the
long-standing impact on the API. Where I would need to explain that the
`NoInit` value does not mean that:
* no initializer was written (because it is also Implicit)
* no initialization was performed (because `Implicit` also means that).
https://github.com/llvm/llvm-project/pull/71417
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
