[clang][NFC] Refactor CXXNewExpr::InitializationStyle (re-land)
#71417
Conversation
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.
That one was tricky to find.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-modules Author: Vlad Serebrennikov (Endilll) ChangesThis 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 #71322 Full diff: https://github.com/llvm/llvm-project/pull/71417.diff 8 Files Affected:
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<Expr *> PlacementArgs,
SourceRange TypeIdParens, std::optional<Expr *> 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<Expr *> PlacementArgs,
SourceRange TypeIdParens, std::optional<Expr *> 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 CXXNewInitializationStyle::List:
+ return true;
+ }
}
/// The kind of initializer this new-expression has.
- InitializationStyle getInitializationStyle() const {
- if (CXXNewExprBits.StoredInitializationStyle == 0)
- return NoInit;
- return static_cast<InitializationStyle>(
- CXXNewExprBits.StoredInitializationStyle - 1);
+ CXXNewInitializationStyle getInitializationStyle() const {
+ return static_cast<CXXNewInitializationStyle>(
+ CXXNewExprBits.StoredInitializationStyle);
}
/// The initializer of this new-expression.
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index 4d2e0e9a945a781..83af7998f683382 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -184,7 +184,7 @@ CXXNewExpr::CXXNewExpr(bool IsGlobalNew, FunctionDecl *OperatorNew,
bool UsualArrayDeleteWantsSize,
ArrayRef<Expr *> PlacementArgs, SourceRange TypeIdParens,
std::optional<Expr *> ArraySize,
- InitializationStyle InitializationStyle,
+ CXXNewInitializationStyle InitializationStyle,
Expr *Initializer, QualType Ty,
TypeSourceInfo *AllocatedTypeInfo, SourceRange Range,
SourceRange DirectInitRange)
@@ -193,7 +193,9 @@ CXXNewExpr::CXXNewExpr(bool IsGlobalNew, FunctionDecl *OperatorNew,
AllocatedTypeInfo(AllocatedTypeInfo), Range(Range),
DirectInitRange(DirectInitRange) {
- assert((Initializer != nullptr || InitializationStyle == NoInit) &&
+ assert((Initializer != nullptr ||
+ InitializationStyle == CXXNewInitializationStyle::None ||
+ InitializationStyle == CXXNewInitializationStyle::Implicit) &&
"Only NoInit can have no initializer!");
CXXNewExprBits.IsGlobalNew = IsGlobalNew;
@@ -201,7 +203,7 @@ CXXNewExpr::CXXNewExpr(bool IsGlobalNew, FunctionDecl *OperatorNew,
CXXNewExprBits.ShouldPassAlignment = ShouldPassAlignment;
CXXNewExprBits.UsualArrayDeleteWantsSize = UsualArrayDeleteWantsSize;
CXXNewExprBits.StoredInitializationStyle =
- Initializer ? InitializationStyle + 1 : 0;
+ llvm::to_underlying(InitializationStyle);
bool IsParenTypeId = TypeIdParens.isValid();
CXXNewExprBits.IsParenTypeId = IsParenTypeId;
CXXNewExprBits.NumPlacementArgs = PlacementArgs.size();
@@ -217,10 +219,10 @@ CXXNewExpr::CXXNewExpr(bool IsGlobalNew, FunctionDecl *OperatorNew,
getTrailingObjects<SourceRange>()[0] = TypeIdParens;
switch (getInitializationStyle()) {
- case CallInit:
+ case CXXNewInitializationStyle::Call:
this->Range.setEnd(DirectInitRange.getEnd());
break;
- case ListInit:
+ case CXXNewInitializationStyle::List:
this->Range.setEnd(getInitializer()->getSourceRange().getEnd());
break;
default:
@@ -240,15 +242,14 @@ CXXNewExpr::CXXNewExpr(EmptyShell Empty, bool IsArray,
CXXNewExprBits.IsParenTypeId = IsParenTypeId;
}
-CXXNewExpr *
-CXXNewExpr::Create(const ASTContext &Ctx, bool IsGlobalNew,
- FunctionDecl *OperatorNew, FunctionDecl *OperatorDelete,
- bool ShouldPassAlignment, bool UsualArrayDeleteWantsSize,
- ArrayRef<Expr *> PlacementArgs, SourceRange TypeIdParens,
- std::optional<Expr *> ArraySize,
- InitializationStyle InitializationStyle, Expr *Initializer,
- QualType Ty, TypeSourceInfo *AllocatedTypeInfo,
- SourceRange Range, SourceRange DirectInitRange) {
+CXXNewExpr *CXXNewExpr::Create(
+ const ASTContext &Ctx, bool IsGlobalNew, FunctionDecl *OperatorNew,
+ FunctionDecl *OperatorDelete, bool ShouldPassAlignment,
+ bool UsualArrayDeleteWantsSize, ArrayRef<Expr *> PlacementArgs,
+ SourceRange TypeIdParens, std::optional<Expr *> ArraySize,
+ CXXNewInitializationStyle InitializationStyle, Expr *Initializer,
+ QualType Ty, TypeSourceInfo *AllocatedTypeInfo, SourceRange Range,
+ SourceRange DirectInitRange) {
bool IsArray = ArraySize.has_value();
bool HasInit = Initializer != nullptr;
unsigned NumPlacementArgs = PlacementArgs.size();
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 8530675ca2a1ce2..5ac8c2e447cdb5a 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -4826,7 +4826,7 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity,
Out << '_';
mangleType(New->getAllocatedType());
if (New->hasInitializer()) {
- if (New->getInitializationStyle() == CXXNewExpr::ListInit)
+ if (New->getInitializationStyle() == CXXNewInitializationStyle::List)
Out << "il";
else
Out << "pi";
@@ -4840,7 +4840,8 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity,
} else if (const ParenListExpr *PLE = dyn_cast<ParenListExpr>(Init)) {
for (unsigned i = 0, e = PLE->getNumExprs(); i != e; ++i)
mangleExpression(PLE->getExpr(i));
- } else if (New->getInitializationStyle() == CXXNewExpr::ListInit &&
+ } else if (New->getInitializationStyle() ==
+ CXXNewInitializationStyle::List &&
isa<InitListExpr>(Init)) {
// Only take InitListExprs apart for list-initialization.
mangleInitListElements(cast<InitListExpr>(Init));
diff --git a/clang/lib/AST/JSONNodeDumper.cpp b/clang/lib/AST/JSONNodeDumper.cpp
index 1a013b45c615d1b..bc7bc7337b15e92 100644
--- a/clang/lib/AST/JSONNodeDumper.cpp
+++ b/clang/lib/AST/JSONNodeDumper.cpp
@@ -1351,9 +1351,15 @@ void JSONNodeDumper::VisitCXXNewExpr(const CXXNewExpr *NE) {
attributeOnlyIfTrue("isArray", NE->isArray());
attributeOnlyIfTrue("isPlacement", NE->getNumPlacementArgs() != 0);
switch (NE->getInitializationStyle()) {
- case CXXNewExpr::NoInit: break;
- case CXXNewExpr::CallInit: JOS.attribute("initStyle", "call"); break;
- case CXXNewExpr::ListInit: JOS.attribute("initStyle", "list"); break;
+ case CXXNewInitializationStyle::None:
+ case CXXNewInitializationStyle::Implicit:
+ break;
+ case CXXNewInitializationStyle::Call:
+ JOS.attribute("initStyle", "call");
+ break;
+ case CXXNewInitializationStyle::List:
+ JOS.attribute("initStyle", "list");
+ break;
}
if (const FunctionDecl *FD = NE->getOperatorNew())
JOS.attribute("operatorNewDecl", createBareDeclRef(FD));
diff --git a/clang/lib/AST/StmtPrinter.cpp b/clang/lib/AST/StmtPrinter.cpp
index a31aa0cfeeed8de..1e04bfdbab32445 100644
--- a/clang/lib/AST/StmtPrinter.cpp
+++ b/clang/lib/AST/StmtPrinter.cpp
@@ -2298,9 +2298,10 @@ void StmtPrinter::VisitCXXNewExpr(CXXNewExpr *E) {
if (E->isParenTypeId())
OS << ")";
- CXXNewExpr::InitializationStyle InitStyle = E->getInitializationStyle();
- if (InitStyle != CXXNewExpr::NoInit) {
- bool Bare = InitStyle == CXXNewExpr::CallInit &&
+ CXXNewInitializationStyle InitStyle = E->getInitializationStyle();
+ if (InitStyle != CXXNewInitializationStyle::None &&
+ InitStyle != CXXNewInitializationStyle::Implicit) {
+ bool Bare = InitStyle == CXXNewInitializationStyle::Call &&
!isa<ParenListExpr>(E->getInitializer());
if (Bare)
OS << "(";
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 6510fa369d78eb6..8128219dd6f63c9 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2096,7 +2096,7 @@ void StmtProfiler::VisitCXXNewExpr(const CXXNewExpr *S) {
ID.AddInteger(S->getNumPlacementArgs());
ID.AddBoolean(S->isGlobalNew());
ID.AddBoolean(S->isParenTypeId());
- ID.AddInteger(S->getInitializationStyle());
+ ID.AddInteger(llvm::to_underlying(S->getInitializationStyle()));
}
void
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 25d7759cc168dd4..c07462aa2444403 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1946,7 +1946,7 @@ Sema::ActOnCXXNew(SourceLocation StartLoc, bool UseGlobal,
Initializer);
}
-static bool isLegalArrayNewInitializer(CXXNewExpr::InitializationStyle Style,
+static bool isLegalArrayNewInitializer(CXXNewInitializationStyle Style,
Expr *Init) {
if (!Init)
return true;
@@ -1957,7 +1957,7 @@ static bool isLegalArrayNewInitializer(CXXNewExpr::InitializationStyle Style,
else if (CXXConstructExpr *CCE = dyn_cast<CXXConstructExpr>(Init))
return !CCE->isListInitialization() &&
CCE->getConstructor()->isDefaultConstructor();
- else if (Style == CXXNewExpr::ListInit) {
+ else if (Style == CXXNewInitializationStyle::List) {
assert(isa<InitListExpr>(Init) &&
"Shouldn't create list CXXConstructExprs for arrays.");
return true;
@@ -2008,44 +2008,49 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
SourceRange TypeRange = AllocTypeInfo->getTypeLoc().getSourceRange();
SourceLocation StartLoc = Range.getBegin();
- CXXNewExpr::InitializationStyle initStyle;
+ CXXNewInitializationStyle InitStyle;
if (DirectInitRange.isValid()) {
assert(Initializer && "Have parens but no initializer.");
- initStyle = CXXNewExpr::CallInit;
+ InitStyle = CXXNewInitializationStyle::Call;
} else if (Initializer && isa<InitListExpr>(Initializer))
- initStyle = CXXNewExpr::ListInit;
+ InitStyle = CXXNewInitializationStyle::List;
else {
assert((!Initializer || isa<ImplicitValueInitExpr>(Initializer) ||
isa<CXXConstructExpr>(Initializer)) &&
"Initializer expression that cannot have been implicitly created.");
- initStyle = CXXNewExpr::NoInit;
+ InitStyle = CXXNewInitializationStyle::None;
}
MultiExprArg Exprs(&Initializer, Initializer ? 1 : 0);
if (ParenListExpr *List = dyn_cast_or_null<ParenListExpr>(Initializer)) {
- assert(initStyle == CXXNewExpr::CallInit && "paren init for non-call init");
+ assert(InitStyle == CXXNewInitializationStyle::Call &&
+ "paren init for non-call init");
Exprs = MultiExprArg(List->getExprs(), List->getNumExprs());
}
// C++11 [expr.new]p15:
// A new-expression that creates an object of type T initializes that
// object as follows:
- InitializationKind Kind
- // - If the new-initializer is omitted, the object is default-
- // initialized (8.5); if no initialization is performed,
- // the object has indeterminate value
- = initStyle == CXXNewExpr::NoInit
- ? InitializationKind::CreateDefault(TypeRange.getBegin())
- // - Otherwise, the new-initializer is interpreted according to
- // the
- // initialization rules of 8.5 for direct-initialization.
- : initStyle == CXXNewExpr::ListInit
- ? InitializationKind::CreateDirectList(
- TypeRange.getBegin(), Initializer->getBeginLoc(),
- Initializer->getEndLoc())
- : InitializationKind::CreateDirect(TypeRange.getBegin(),
- DirectInitRange.getBegin(),
- DirectInitRange.getEnd());
+ InitializationKind Kind = [&] {
+ switch (InitStyle) {
+ // - If the new-initializer is omitted, the object is default-
+ // initialized (8.5); if no initialization is performed,
+ // the object has indeterminate value
+ case CXXNewInitializationStyle::None:
+ case CXXNewInitializationStyle::Implicit:
+ return InitializationKind::CreateDefault(TypeRange.getBegin());
+ // - Otherwise, the new-initializer is interpreted according to the
+ // initialization rules of 8.5 for direct-initialization.
+ case CXXNewInitializationStyle::Call:
+ return InitializationKind::CreateDirect(TypeRange.getBegin(),
+ DirectInitRange.getBegin(),
+ DirectInitRange.getEnd());
+ case CXXNewInitializationStyle::List:
+ return InitializationKind::CreateDirectList(TypeRange.getBegin(),
+ Initializer->getBeginLoc(),
+ Initializer->getEndLoc());
+ }
+ }();
// C++11 [dcl.spec.auto]p6. Deduce the type which 'auto' stands in for.
auto *Deduced = AllocType->getContainedDeducedType();
@@ -2066,13 +2071,14 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
return ExprError();
} else if (Deduced && !Deduced->isDeduced()) {
MultiExprArg Inits = Exprs;
- bool Braced = (initStyle == CXXNewExpr::ListInit);
+ bool Braced = (InitStyle == CXXNewInitializationStyle::List);
if (Braced) {
auto *ILE = cast<InitListExpr>(Exprs[0]);
Inits = MultiExprArg(ILE->getInits(), ILE->getNumInits());
}
- if (initStyle == CXXNewExpr::NoInit || Inits.empty())
+ if (InitStyle == CXXNewInitializationStyle::None ||
+ InitStyle == CXXNewInitializationStyle::Implicit || Inits.empty())
return ExprError(Diag(StartLoc, diag::err_auto_new_requires_ctor_arg)
<< AllocType << TypeRange);
if (Inits.size() > 1) {
@@ -2396,7 +2402,7 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
// Array 'new' can't have any initializers except empty parentheses.
// Initializer lists are also allowed, in C++11. Rely on the parser for the
// dialect distinction.
- if (ArraySize && !isLegalArrayNewInitializer(initStyle, Initializer)) {
+ if (ArraySize && !isLegalArrayNewInitializer(InitStyle, Initializer)) {
SourceRange InitRange(Exprs.front()->getBeginLoc(),
Exprs.back()->getEndLoc());
Diag(StartLoc, diag::err_new_array_init_args) << InitRange;
@@ -2436,6 +2442,9 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
FullInit = Binder->getSubExpr();
Initializer = FullInit.get();
+ if (InitStyle == CXXNewInitializationStyle::None && Initializer) {
+ InitStyle = CXXNewInitializationStyle::Implicit;
+ }
// FIXME: If we have a KnownArraySize, check that the array bound of the
// initializer is no greater than that constant value.
@@ -2468,7 +2477,7 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
return CXXNewExpr::Create(Context, UseGlobal, OperatorNew, OperatorDelete,
PassAlignment, UsualArrayDeleteWantsSize,
- PlacementArgs, TypeIdParens, ArraySize, initStyle,
+ PlacementArgs, TypeIdParens, ArraySize, InitStyle,
Initializer, ResultType, AllocTypeInfo, Range,
DirectInitRange);
}
|
|
The commit "[clang][NFC] Refactor diff --git a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
index 8f0dd5602307..59a1a673dd99 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -2738,6 +2738,35 @@ TEST(MatchFinderAPI, MatchesDynamic) {
EXPECT_EQ(MethodNode, GlobalMethodNode);
}
+TEST(MatchFinderAPI, InvalidParenOrBraceRange) {
+ StringRef SourceCode = R"cpp(
+struct Base {
+ Base(int x, int y, int z);
+};
+class Derived : public Base {
+public:
+ Derived();
+ explicit Derived(int x);
+};
+void top() {
+ const auto *p = new Derived;
+}
+ )cpp";
+
+ // master: getInitializationStyle() == CXXNewExpr::NoInit
+
+ auto Matcher = cxxNewExpr(has(cxxConstructExpr())).bind("target");
+ auto AstUnit = tooling::buildASTFromCode(SourceCode);
+ auto Matches = matchDynamic(Matcher, AstUnit->getASTContext());
+ const auto &SM = AstUnit->getSourceManager();
+ ASSERT_EQ(Matches.size(), 1u);
+ ASSERT_EQ(Matches[0].getMap().size(), 1u);
+ const auto *NewExpr = Matches[0].getNodeAs<CXXNewExpr>("target");
+ ASSERT_TRUE(NewExpr != nullptr);
+ int Actual = (int)NewExpr->getInitializationStyle();
+ ASSERT_EQ(0, Actual);
+}
+
static std::vector<TestClangConfig> allTestClangConfigs() {
std::vector<TestClangConfig> all_configs;
for (TestLanguage lang : {Lang_C89, Lang_C99, Lang_CXX03, Lang_CXX11,The assertion Reproducer instructions after applying the patch: ninja -C build/rel ASTMatchersTests
./build/rel/tools/clang/unittests/ASTMatchers/ASTMatchersTests --gtest_filter=MatchFinderAPI.InvalidParenOrBraceRangeI'd qualify this as a regression, by looking at that the commit was supposed to be an NFC. |
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 |
|
@steakhal Thanks for raising this. I agree this stretches the definition of NFC commit. 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! |
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. |
I'm not really well versed about c++ initialization, so I asked my collage @tomasz-kaminski-sonarsource, to double-check how The enum we had in the past described the syntax of the new expression. However, after the introduction of the Note that the name of the previous kind To illustrate this, here is a small example: 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
} |
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: CXXNewExprBits.StoredInitializationStyle =
Initializer ? InitializationStyle + 1 : 0; 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 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 If have found a previous state, where this enumeration corresponded to what was present in code, much cleaner, where the meaning was clear:
If we introduce In short, as the downstream that uses AST for writing rules, we will need to update all the uses of |
|
Would it be helpful for you if we:
|
|
And what will the desired use of having the distinction I would be very supportive of updating the names, so we replace But this PR added a new enum value that is equivalent to |
|
What I don't want to lose from this patch are the changes to places like
There is an implicit constructor call but it's a noop because the type is trivial, so I think 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
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.) |
|
Reworked response to provide more constructive feedback.
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 switch (cxxNew.getWrittenInitializationStyle()) {
case NoInit:
/* .... */;
case Call:
/* .... */;
case List:
/* .... */
}
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 And we in the downstream, would need to do the same as was done in LLVM find all uses of |
|
I had an offline discussion with @Endilll during my morning office hours today, and our current plan is:
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. |
This is a follow up to llvm#71417 , which aims to resolve concerns brought up there. Namely, this patch replaces `CXXNewInitializationStyle::Implicit` with a dedicated `HasInitializer` flag. This makes `CXXNewInitializationStyle` to model syntax again. This patch also renames `Call` and `List` to less confusing `Parens` and `Braces`.
Thank you for raising the concern, I think we ended up with a cleaner solution in the end! |
This is a follow up to #71417 , which aims to resolve concerns brought up there. Namely, this patch replaces `CXXNewInitializationStyle::Implicit` with a dedicated `HasInitializer` flag. This makes `CXXNewInitializationStyle` to model syntax again. This patch also renames `Call` and `List` to less confusing `Parens` and `Braces`.
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 #71322