Skip to content

[clang][NFC] Refactor CXXNewExpr::InitializationStyle #71322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

Endilll
Copy link
Contributor

@Endilll Endilll commented Nov 5, 2023

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 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.
@Endilll Endilll requested a review from AaronBallman November 5, 2023 16:00
@Endilll Endilll changed the title [clang][NFC] Refacator CXXNewExpr::InitializationStyle [clang][NFC] Refactor CXXNewExpr::InitializationStyle Nov 5, 2023
@Endilll Endilll marked this pull request as ready for review November 6, 2023 13:31
@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tidy clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Nov 6, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 6, 2023

@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-tidy

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.


Full diff: https://github.com/llvm/llvm-project/pull/71322.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 (+33-27)
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..d947aba70d8f362 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;
@@ -2468,7 +2474,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);
 }

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Endilll Endilll merged commit ace4489 into llvm:main Nov 6, 2023
Endilll added a commit that referenced this pull request Nov 6, 2023
@zmodem
Copy link
Collaborator

zmodem commented Nov 6, 2023

I landed a commit close to this one, and got at least 10 emails about broken buildbots so far (e.g. https://lab.llvm.org/buildbot/#/builders/188/builds/37563). How come presubmit testing isn't catching this? @metaflow

@PiotrZSL
Copy link
Member

PiotrZSL commented Nov 6, 2023

It catches (https://buildkite.com/llvm-project/github-pull-requests/builds/12931#018ba4b7-1e05-425f-a30d-46ac33f582b6), you just didn't wait for a results (or ignored them) and forced a merge.

@Endilll
Copy link
Contributor Author

Endilll commented Nov 6, 2023

I've seen those exact test failures locally, but since precommit CI was fine, I landed this PR and kept a close eye on the bots.
Me and Aaron are also wondering now how did it pass CI, and what's wrong with changes here.
I'm sorry @zmodem that we got you involved.

It catches (https://buildkite.com/llvm-project/github-pull-requests/builds/12931#018ba4b7-1e05-425f-a30d-46ac33f582b6), you just didn't wait for a results (or ignored them) and forced a merge.

Me and Aaron seen them, and deemed them unrelated. For a comparison, here is a list of tests that fails locally for me (and post-commit bots seem to agree):

Failed Tests (49):
  Clang :: Analysis/NewDelete-checker-test.cpp
  Clang :: Analysis/bstring.cpp
  Clang :: Analysis/cfg.cpp
  Clang :: Analysis/ctor-array.cpp
  Clang :: Analysis/cxxctr-array-evalcall-analysis-order.cpp
  Clang :: Analysis/dtor-array.cpp
  Clang :: Analysis/dump_egraph.cpp
  Clang :: Analysis/exploded-graph-rewriter/dynamic_types.cpp
  Clang :: Analysis/flexible-array-member.cpp
  Clang :: Analysis/handle_constructors_with_new_array.cpp
  Clang :: Analysis/more-dtors-cfg-output.cpp
  Clang :: Analysis/new-ctor-conservative.cpp
  Clang :: Analysis/new-ctor-inlined.cpp
  Clang :: Analysis/this-pointer.cpp
  Clang :: CXX/dcl.dcl/dcl.spec/dcl.constexpr/p9.cpp
  Clang :: CXX/drs/dr1748.cpp
  Clang :: CodeGen/available-externally-hidden.cpp
  Clang :: CodeGen/debug-prefix-map.cpp
  Clang :: CodeGen/split-lto-unit-input.cpp
  Clang :: CodeGen/tbaa-for-vptr.cpp
  Clang :: CodeGenCUDA/member-init.cu
  Clang :: CodeGenCXX/attr-disable-tail-calls.cpp
  Clang :: CodeGenCXX/catch-undef-behavior.cpp
  Clang :: CodeGenCXX/cfi-ms-vbase-derived-cast.cpp
  Clang :: CodeGenCXX/code-seg.cpp
  Clang :: CodeGenCXX/ctor-dtor-alias.cpp
  Clang :: CodeGenCXX/cxx1z-aligned-allocation.cpp
  Clang :: CodeGenCXX/cxx2a-destroying-delete.cpp
  Clang :: CodeGenCXX/default-arguments.cpp
  Clang :: CodeGenCXX/destructors.cpp
  Clang :: CodeGenCXX/exceptions.cpp
  Clang :: CodeGenCXX/invariant.group-for-vptrs.cpp
  Clang :: CodeGenCXX/key-function-vtable.cpp
  Clang :: CodeGenCXX/microsoft-abi-structors-delayed-template.cpp
  Clang :: CodeGenCXX/new-overflow.cpp
  Clang :: CodeGenCXX/new.cpp
  Clang :: CodeGenCXX/static-init.cpp
  Clang :: CodeGenCXX/strict-vtable-pointers.cpp
  Clang :: CodeGenCXX/type-metadata-thinlto.cpp
  Clang :: CodeGenCXX/ubsan-new-checks.cpp
  Clang :: CodeGenCXX/ubsan-suppress-checks.cpp
  Clang :: CodeGenCXX/vtable-assume-load-address-space.cpp
  Clang :: CodeGenCXX/vtable-assume-load.cpp
  Clang :: CodeGenCXX/vtable-available-externally.cpp
  Clang :: CodeGenObjCXX/arc-new-delete.mm
  Clang :: CodeGenObjCXX/destroy.mm
  Clang :: LibClang/symbols.test
  Clang :: SemaCXX/constant-expression-cxx2a.cpp
  Clang :: SemaCXX/constexpr-turing-cxx2a.cpp

Endilll added a commit that referenced this pull request Nov 6, 2023
…1417)

This patch converts CXXNewExpr::InitializationStyle into a scoped enumat namespace scope. It also affirms the status quo by adding a new enumerator to represent implicit initializer.

This is a re-land of #71322 ace4489
@zmodem
Copy link
Collaborator

zmodem commented Nov 7, 2023

Me and Aaron are also wondering now how did it pass CI, and what's wrong with changes here. I'm sorry @zmodem that we got you involved.

Just to be clear, my comment wasn't intended as criticism of this patch. This does seem like a good opportunity to improve our presubmit testing though; filed #71532 .

@AaronBallman
Copy link
Collaborator

Me and Aaron are also wondering now how did it pass CI, and what's wrong with changes here. I'm sorry @zmodem that we got you involved.

Just to be clear, my comment wasn't intended as criticism of this patch. This does seem like a good opportunity to improve our presubmit testing though; filed #71532 .

Thank you for filing that! I had looked through the precommit CI when accepting the patch and the clang-tools-extra failures appeared to be unrelated and I never saw any other failures reported. It could be that the clang-tools-extra failures managed to hide the clang failures, it could be a Ux issue where I simply missed the clang failures, or it could be an issue where the failures never got reported. (I don't believe it was possible for the Clang CI to be green with those changes as they were.)

@joker-eph
Copy link
Collaborator

I've seen those exact test failures locally, but since precommit CI was fine, I landed this PR and kept a close eye on the bots.

If you see failures locally, it's best to understand where they coming from: even if it passes on bots it just means we have a hole in our test coverage.
For the pre-commit CI, one thing to look for is a "positive signal" instead of the absence of "negative signal": that is checking in the log for a "success" for a given tests (depending on the config, tests can be marked "unsupported" or not executed for various reasons).
Thanks @zmodem for reporting the issue: we need to fix the premerge config!

@Endilll
Copy link
Contributor Author

Endilll commented Nov 7, 2023

If you see failures locally, it's best to understand where they coming from: even if it passes on bots it just means we have a hole in our test coverage.

Sure, but it wasn't the first time I've seen local test failures that doesn't reproduce anywhere else (Clang :: LibClang/symbols.test). By the time I merged this PR, me and Aaron have spent hours staring at the changes without any progress. So we wanted an additional confirmation that this is not in the same as Clang :: LibClang/symbols.test failure, especially looking at pre-commit CI that reported just a several seemingly unrelated clang-tidy failures. I hope it is acceptable that we decided not to spend even more time before trying this PR out on wider range of buildbots.

@AaronBallman
Copy link
Collaborator

If you see failures locally, it's best to understand where they coming from: even if it passes on bots it just means we have a hole in our test coverage.

Sure, but it wasn't the first time I've seen local test failures that doesn't reproduce anywhere else (Clang :: LibClang/symbols.test). By the time I merged this PR, me and Aaron have spent hours staring at the changes without any progress. So we wanted an additional confirmation that this is not in the same as Clang :: LibClang/symbols.test failure, especially looking at pre-commit CI that reported just a several seemingly unrelated clang-tidy failures. I hope it is acceptable that we decided not to spend even more time before trying this PR out on wider range of buildbots.

I think everyone is correct here and we're all on the same page. Typically, local failures mean the code isn't correct so it's not ready to commit. However, one-off circumstances do happen where tests will fail locally but pass everywhere else (for example, people working on slightly out-of-norm configurations will sometimes have persistent local failures that are unrelated to changes in the patch). When precommit CI comes back green or with only false positives and you've got such local failures, speculative commits to see whether an issue "is real" or not do happen on occasion and are appropriate so long as necessary follow-up actions are timely (which they were in this case).

@joker-eph
Copy link
Collaborator

tests will fail locally but pass everywhere else (for example, people working on slightly out-of-norm configurations will sometimes have persistent local failures that are unrelated to changes in the patch).

It never happened to me, in a way that can't be root caused and fixed (that is for example cases like that would be the name of my directory interfere with a CHECK somehow), what do you have in mind?

When precommit CI comes back green or with only false positives

The point of my message was that the CI wasn't a false positive: the CI didn't run. A false positive would be seeing a "Passed" for the test that you see failing locally.

speculative commits to see whether an issue "is real" or not do happen on occasion and are appropriate

The cases I have seen for "speculative commits" are the opposite of what you're describing: iterating on issues that only happens on a bot and not reproducible locally. That is you have to push a commit because this is the only way to iterate on debugging an issue.
Pushing a commit when you have a local failure seems backward to me, I don't quite follow actually.

@AaronBallman
Copy link
Collaborator

tests will fail locally but pass everywhere else (for example, people working on slightly out-of-norm configurations will sometimes have persistent local failures that are unrelated to changes in the patch).

It never happened to me, in a way that can't be root caused and fixed (that is for example cases like that would be the name of my directory interfere with a CHECK somehow), what do you have in mind?

I do most of my work on Windows which is not the best-tested platform we have. I've frequently had failures locally of the variety "someone committed code that causes an MSVC STL debug iterator assertion", "mingw behaves differently than cygwin which behaves differently than Windows", git's autocrlf was not set properly, AV software impacting test behavior, tests hitting timeouts, etc. @Endilll has one locally where libclang symbols test fails for him in inexplicable ways that don't reproduce on any bots. That sort of thing.

When precommit CI comes back green or with only false positives

The point of my message was that the CI wasn't a false positive: the CI didn't run. A false positive would be seeing a "Passed" for the test that you see failing locally.

Except CI did run and we got failure reports that were believed to be false positives in clang-tools-extra. The only way we could notice that Clang tests did not run was to see:

Total Discovered Tests: 2777
  Unsupported      :    7 (0.25%)
  Passed           : 2765 (99.57%)
  Expectedly Failed:    2 (0.07%)
  Failed           :    3 (0.11%)

and realize that's a very small number of tests. That's not a particularly obvious signal for code reviewers and it's even less obvious to newer contributors.

speculative commits to see whether an issue "is real" or not do happen on occasion and are appropriate

The cases I have seen for "speculative commits" are the opposite of what you're describing: iterating on issues that only happens on a bot and not reproducible locally. That is you have to push a commit because this is the only way to iterate on debugging an issue. Pushing a commit when you have a local failure seems backward to me, I don't quite follow actually.

That is definitely the more common scenario, to be sure. But local failures that are not reproduced on bots are a reality unfortunately. Obviously, the goal is for tests to pass everywhere before committing.

Endilll added a commit that referenced this pull request Nov 7, 2023
It was an unfortunate oversight from my side to forget to include this comment into the PR.
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
…1417)

This patch converts CXXNewExpr::InitializationStyle into a scoped enumat namespace scope. It also affirms the status quo by adding a new enumerator to represent implicit initializer.

This is a re-land of llvm/llvm-project#71322 ace4489
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category clang-tidy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants