[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
https://github.com/Endilll created
https://github.com/llvm/llvm-project/pull/71417
This patch converts CXXNewExpr::InitializationStyle into a scoped enum at
namespace scope. It also affirms the status quo by adding a new enumerator to
represent implicit initializer.
This is a re-land of https://github.com/llvm/llvm-project/pull/71322
>From 40d25b8009f1c8734a99fd1350adaced6884cc7f Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov
Date: Sun, 5 Nov 2023 18:53:48 +0300
Subject: [PATCH 1/6] [clang][NFC] Refacator `CXXNewExpr::InitializationStyle`
This patch converts `CXXNewExpr::InitializationStyle` into a scoped enum at
namespace scope. It also affirms the status quo by adding a new enumerator to
represent implicit initializer.
---
.../modernize/MakeSmartPtrCheck.cpp | 7 +--
clang/include/clang/AST/ExprCXX.h | 48 +++
clang/lib/AST/ExprCXX.cpp | 12 ++---
clang/lib/AST/ItaniumMangle.cpp | 4 +-
clang/lib/AST/JSONNodeDumper.cpp | 7 +--
clang/lib/AST/StmtPrinter.cpp | 6 +--
clang/lib/AST/StmtProfile.cpp | 2 +-
clang/lib/Sema/SemaExprCXX.cpp| 22 -
8 files changed, 58 insertions(+), 50 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
b/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
index 71fd8eca300c1b2..616e57efa76ded5 100644
--- a/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -323,7 +323,8 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
return false;
};
switch (New->getInitializationStyle()) {
- case CXXNewExpr::NoInit: {
+ case CXXNewInitializationStyle::None:
+ case CXXNewInitializationStyle::Implicit: {
if (ArraySizeExpr.empty()) {
Diag << FixItHint::CreateRemoval(SourceRange(NewStart, NewEnd));
} else {
@@ -334,7 +335,7 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
}
break;
}
- case CXXNewExpr::CallInit: {
+ case CXXNewInitializationStyle::Call: {
// FIXME: Add fixes for constructors with parameters that can be created
// with a C++11 braced-init-list (e.g. std::vector, std::map).
// Unlike ordinal cases, braced list can not be deduced in
@@ -371,7 +372,7 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
}
break;
}
- case CXXNewExpr::ListInit: {
+ case CXXNewInitializationStyle::List: {
// Range of the substring that we do not want to remove.
SourceRange InitRange;
if (const auto *NewConstruct = New->getConstructExpr()) {
diff --git a/clang/include/clang/AST/ExprCXX.h
b/clang/include/clang/AST/ExprCXX.h
index a106bafcfa3e021..d713bcf8eb70258 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -2206,6 +2206,20 @@ class CXXScalarValueInitExpr : public Expr {
}
};
+enum class CXXNewInitializationStyle {
+ /// New-expression has no initializer as written.
+ None,
+
+ /// New-expression has no written initializer, but has an implicit one.
+ Implicit,
+
+ /// New-expression has a C++98 paren-delimited initializer.
+ Call,
+
+ /// New-expression has a C++11 list-initializer.
+ List
+};
+
/// Represents a new-expression for memory allocation and constructor
/// calls, e.g: "new CXXNewExpr(foo)".
class CXXNewExpr final
@@ -2259,25 +2273,12 @@ class CXXNewExpr final
return isParenTypeId();
}
-public:
- enum InitializationStyle {
-/// New-expression has no initializer as written.
-NoInit,
-
-/// New-expression has a C++98 paren-delimited initializer.
-CallInit,
-
-/// New-expression has a C++11 list-initializer.
-ListInit
- };
-
-private:
/// Build a c++ new expression.
CXXNewExpr(bool IsGlobalNew, FunctionDecl *OperatorNew,
FunctionDecl *OperatorDelete, bool ShouldPassAlignment,
bool UsualArrayDeleteWantsSize, ArrayRef PlacementArgs,
SourceRange TypeIdParens, std::optional ArraySize,
- InitializationStyle InitializationStyle, Expr *Initializer,
+ CXXNewInitializationStyle InitializationStyle, Expr *Initializer,
QualType Ty, TypeSourceInfo *AllocatedTypeInfo, SourceRange Range,
SourceRange DirectInitRange);
@@ -2292,7 +2293,7 @@ class CXXNewExpr final
FunctionDecl *OperatorDelete, bool ShouldPassAlignment,
bool UsualArrayDeleteWantsSize, ArrayRef PlacementArgs,
SourceRange TypeIdParens, std::optional ArraySize,
- InitializationStyle InitializationStyle, Expr *Initializer,
+ CXXNewInitializationStyle InitializationStyle, Expr *Initializer,
QualType Ty, TypeSourceInfo *AllocatedTypeInfo, SourceRange Range,
SourceRange DirectInitRange);
@@ -2388,15 +2389,20 @@ class CXXNewExpr final
/// Whether this new-expression has any initiali
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
llvmbot wrote:
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-modules
Author: Vlad Serebrennikov (Endilll)
Changes
This patch converts CXXNewExpr::InitializationStyle into a scoped enum at
namespace scope. It also affirms the status quo by adding a new enumerator to
represent implicit initializer.
This is a re-land of https://github.com/llvm/llvm-project/pull/71322
---
Full diff: https://github.com/llvm/llvm-project/pull/71417.diff
8 Files Affected:
- (modified) clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
(+4-3)
- (modified) clang/include/clang/AST/ExprCXX.h (+27-21)
- (modified) clang/lib/AST/ExprCXX.cpp (+15-14)
- (modified) clang/lib/AST/ItaniumMangle.cpp (+3-2)
- (modified) clang/lib/AST/JSONNodeDumper.cpp (+9-3)
- (modified) clang/lib/AST/StmtPrinter.cpp (+4-3)
- (modified) clang/lib/AST/StmtProfile.cpp (+1-1)
- (modified) clang/lib/Sema/SemaExprCXX.cpp (+36-27)
``diff
diff --git a/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
b/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
index 71fd8eca300c1b2..616e57efa76ded5 100644
--- a/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -323,7 +323,8 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
return false;
};
switch (New->getInitializationStyle()) {
- case CXXNewExpr::NoInit: {
+ case CXXNewInitializationStyle::None:
+ case CXXNewInitializationStyle::Implicit: {
if (ArraySizeExpr.empty()) {
Diag << FixItHint::CreateRemoval(SourceRange(NewStart, NewEnd));
} else {
@@ -334,7 +335,7 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
}
break;
}
- case CXXNewExpr::CallInit: {
+ case CXXNewInitializationStyle::Call: {
// FIXME: Add fixes for constructors with parameters that can be created
// with a C++11 braced-init-list (e.g. std::vector, std::map).
// Unlike ordinal cases, braced list can not be deduced in
@@ -371,7 +372,7 @@ bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
}
break;
}
- case CXXNewExpr::ListInit: {
+ case CXXNewInitializationStyle::List: {
// Range of the substring that we do not want to remove.
SourceRange InitRange;
if (const auto *NewConstruct = New->getConstructExpr()) {
diff --git a/clang/include/clang/AST/ExprCXX.h
b/clang/include/clang/AST/ExprCXX.h
index a106bafcfa3e021..37d310ef967d9c0 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -2206,6 +2206,20 @@ class CXXScalarValueInitExpr : public Expr {
}
};
+enum class CXXNewInitializationStyle {
+ /// New-expression has no initializer as written.
+ None,
+
+ /// New-expression has no written initializer, but has an implicit one.
+ Implicit,
+
+ /// New-expression has a C++98 paren-delimited initializer.
+ Call,
+
+ /// New-expression has a C++11 list-initializer.
+ List
+};
+
/// Represents a new-expression for memory allocation and constructor
/// calls, e.g: "new CXXNewExpr(foo)".
class CXXNewExpr final
@@ -2259,25 +2273,12 @@ class CXXNewExpr final
return isParenTypeId();
}
-public:
- enum InitializationStyle {
-/// New-expression has no initializer as written.
-NoInit,
-
-/// New-expression has a C++98 paren-delimited initializer.
-CallInit,
-
-/// New-expression has a C++11 list-initializer.
-ListInit
- };
-
-private:
/// Build a c++ new expression.
CXXNewExpr(bool IsGlobalNew, FunctionDecl *OperatorNew,
FunctionDecl *OperatorDelete, bool ShouldPassAlignment,
bool UsualArrayDeleteWantsSize, ArrayRef PlacementArgs,
SourceRange TypeIdParens, std::optional ArraySize,
- InitializationStyle InitializationStyle, Expr *Initializer,
+ CXXNewInitializationStyle InitializationStyle, Expr *Initializer,
QualType Ty, TypeSourceInfo *AllocatedTypeInfo, SourceRange Range,
SourceRange DirectInitRange);
@@ -2292,7 +2293,7 @@ class CXXNewExpr final
FunctionDecl *OperatorDelete, bool ShouldPassAlignment,
bool UsualArrayDeleteWantsSize, ArrayRef PlacementArgs,
SourceRange TypeIdParens, std::optional ArraySize,
- InitializationStyle InitializationStyle, Expr *Initializer,
+ CXXNewInitializationStyle InitializationStyle, Expr *Initializer,
QualType Ty, TypeSourceInfo *AllocatedTypeInfo, SourceRange Range,
SourceRange DirectInitRange);
@@ -2388,15 +2389,20 @@ class CXXNewExpr final
/// Whether this new-expression has any initializer at all.
bool hasInitializer() const {
-return CXXNewExprBits.StoredInitializationStyle > 0;
+switch (getInitializationStyle()) {
+case CXXNewInitializationStyle::None:
+ return false;
+case CXXNewInitializationStyle::Implicit:
+case CXXNewInitializationStyle::Call:
+case CXXNewInitializati
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
tomasz-kaminski-sonarsource wrote: > I had an offline discussion with @Endilll during my morning office hours > today, and our current plan is: > > 1. Remove `Implicit` from the enumeration, rename `Call` and `List` to > `ParenList` and `BraceList`, respectively > 2. Add a new bit to the bit-field for `CXXNewExpr` to track "has an > initializer" instead of encoding it as in-band information in the > initialization style. > 3. Use that new bit internally in `CXXNewExpr`, update serialization and > whatnot accordingly. > > This should bring us back to the enumeration mapping directly to syntax but > removes the strange in-band nature of how the original enumeration was being > used. Thank you! I really like how this direction, that both keeps the benefit of simplified implementation and makes the enumeration values cleaner. Of course this resolves, all concerns that we have with this change. 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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
Endilll wrote: > I'd qualify this as a regression, by looking at that the commit was supposed > to be an NFC. Could you please confirm @Endilll? I'll leave to @AaronBallman to decide whether this is a functional change, but I can confirm that patch is working as intended, because there is an implicit initialization here `const auto *p = new Derived;`, because `Derived` is a class type. 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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
cor3ntin wrote: @steakhal Thanks for raising this. I agree this stretches the definition of NFC commit. But it was dully reviewed and approved https://github.com/llvm/llvm-project/pull/71322 We usually do not make any guarantees as the stability of the C++ interfaces, so as this test did not exist when this change was committed, I am not sure anything was actually broken. Please let us know if this change is disruptive to you though, thanks! 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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
Endilll wrote: > I agree this stretches the definition of NFC commit. But it was dully reviewed and approved https://github.com/llvm/llvm-project/pull/71322 I agree with this assessment. I think it really started as regular NFC, but then me and Aaron realized that we can get rid of some ugly code if we properly model implicit initialization. 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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
steakhal wrote:
> Please let us know if this change is disruptive to you though, thanks!
I'm not really well versed about c++ initialization, so I asked my collage
@tomasz-kaminski-sonarsource, to double-check how `CXXNewExpr` initialization
is done per standard.
I'll try to summarize what we discussed:
The enum we had in the past described the syntax of the new expression.
However, after the introduction of the `Implicit` style kind, this enum tries
to encode the semantics aspect as well.
Note that the name of the previous kind `CallInit` was misleading, and it
should have been called `ParenInit` denoting that in the spelling `(...)`'s
were used. So, in the past, `InitializationStyle` did not try to encode whether
or not an actual call would be present or not.
To illustrate this, here is a small example:
```c++
struct Derived {
int data1;
int data2;
};
void top() {
// CURRENT STYLE, EXPECTED BEHAVIOR
// - -
// const auto *p = new int(); // Call (aka. parens), good (zero inits)
// const auto *p = new int; // None, good (uninitialized)
// const auto *p = new Derived; // Implicit, but still leaves everything
uninitialized
// const auto *p = new int[10]; // None, good (uninitialized)
// const auto *p = new Derived[10]; // Implicit, but still leaves everything
uninitialized
}
```
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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
Endilll 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:
```cpp
CXXNewExprBits.StoredInitializationStyle =
Initializer ? InitializationStyle + 1 : 0;
```
```cpp
InitializationStyle getInitializationStyle() const {
if (CXXNewExprBits.StoredInitializationStyle == 0)
return NoInit;
return static_cast(
CXXNewExprBits.StoredInitializationStyle - 1);
```
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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
AaronBallman 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(
> 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?
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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
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(
> > 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
AaronBallman 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?
> 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.)
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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
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
[clang] [clang-tools-extra] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
