[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)

via cfe-commits cfe-commits at lists.llvm.org
Thu Jan 18 05:34:53 PST 2024


tomasz-kaminski-sonarsource wrote:

> > > The enum we had in the past described the syntax of the new expression.
> > 
> > 
> > Even if it was the case at some point, I'm not sure it held when I created the PR, which eliminated this kind of nasty mapping, encoding how this enum was actually used:
> > ```c++
> >  CXXNewExprBits.StoredInitializationStyle =
> >       Initializer ? InitializationStyle + 1 : 0;
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > ```c++
> >   InitializationStyle getInitializationStyle() const {
> >     if (CXXNewExprBits.StoredInitializationStyle == 0)
> >       return NoInit;
> >     return static_cast<InitializationStyle>(
> >         CXXNewExprBits.StoredInitializationStyle - 1);
> > ```
> 
> Yeah, perhaps calling this NFC was a stretch because it does have a minor functional change in what it expresses; sorry for that confusion! I think the current form is a more clean way to express the code. Are you finding it's causing issues beyond just the change to test behavior?

I find the new `Implicit` enumeration kind to be confusing, as this value does not indicate that any initialization code will be actually emitted for this new expression. We were trying to demonstrate it with the example of the class with a trivial default constructor:
```c++
struct Trivial {
   int x;
   int y;
};

auto* p1 = new Trivial;
auto* p1 = new Trivial[10];
```
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. This is why I found this value confusing, as it mixes the syntax (is the initializer present) with the semantics of initialization. And the usability of the provided semantic is actually questionable, as it reflects the AST representation of the code (there is `CXXConstructExpr` node that does not call any constructor) versus observable effects of impl.

If have found a previous state, where this enumeration corresponded to what was present in code, much cleaner, where the meaning was clear:
* `NoInit` meant `new X` - regardless if that does nothing or calls the constructor
* `Call` means `new X(...)` - regardless if that value initializes the object with trivial initialization, call constructor, or creates aggregates
* `List` means `new x{....}` - regardless if that value initializes the object with trivial initialization, calls a constructor,  creates aggregates, or produces `std::initializer_list`





https://github.com/llvm/llvm-project/pull/71417


More information about the cfe-commits mailing list