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

[via cfe-commits]

tomasz-kaminski-sonarsource wrote:

Reworked response to provide constructive feedback.

> 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?
> 

It would partially address the issue, however, it is unclear how useful this distinction would be for the outside user of the code. Even if we provided a function that would return `getWrittenInitializationStyle()` that maps `Implicit` to `None`, the presence of the enum will still be exposed, and user will still get a warning about an unhandled enumeration value in the switch. Like following:
```c++
switch (cxxNew.getWrittenInitializationStyle()) {
    case NoInit:
        /* .... */;
     case Call:
       /* .... */;
    case List:
      /* .... */
}
```

> > 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.)

In my opinion (which I know is very subjective), I would not consider this to clarify the code. It removed the storage detail that was present in the `CXXNewExpr`, at the cost of requiring all uses of public `getInitializationStyle()` to handle additional enumeration value, which they do in the same manner as `NoInit` ([uses list](https://github.com/search?q=repo%3Allvm%2Fllvm-project+%22CXXNewInitializationStyle%3A%3ANone%22&type=code)). This lack of distinction on external uses of enumeration, for me suggest that the captured distinction is artificial and just result of implementation details, that would be better keep private.



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