Skip to content
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

[Clang] Implement P2308R1 - Template Parameter Initialization. #73103

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

cor3ntin
Copy link
Contributor

@cor3ntin cor3ntin commented Nov 22, 2023

https://wiki.edg.com/pub/Wg21kona2023/StrawPolls/p2308r1.html

This implements P2308R1 as a DR and resolves CWG2459, CWG2450 and CWG2049.

Fixes #73666
Fixes #58434
Fixes #41227
Fixes #49978
Fixes #36296

@cor3ntin cor3ntin added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Nov 22, 2023
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Nov 22, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 22, 2023

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

https://wiki.edg.com/pub/Wg21kona2023/StrawPolls/p2308r1.html

This implements P2308R1 as a DR and resolves CWG2459, CWG2450 and CWG2049.


Full diff: https://github.com/llvm/llvm-project/pull/73103.diff

11 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/include/clang/Sema/Sema.h (+4)
  • (modified) clang/lib/Parse/ParseTemplate.cpp (+8-3)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+9)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+63-33)
  • (modified) clang/test/CXX/drs/dr20xx.cpp (+8)
  • (added) clang/test/CXX/drs/dr24xx.cpp (+32)
  • (modified) clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp (+3-3)
  • (added) clang/test/SemaTemplate/temp_arg_nontype_cxx2c.cpp (+74)
  • (modified) clang/www/cxx_dr_status.html (+3-3)
  • (modified) clang/www/cxx_status.html (+1-1)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 157afd9e8629152..b3079229c959789 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -182,6 +182,9 @@ C++2c Feature Support
   This is applied to both C++ standard attributes, and other attributes supported by Clang.
   This completes the implementation of `P2361R6 Unevaluated Strings <https://wg21.link/P2361R6>`_
 
+- Implemented `P2361R6 Template parameter initialization <https://wg21.link/P2308R1>`_.
+  This change is applied as a DR in all language modes.
+
 
 Resolutions to C++ Defect Reports
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 59806bcbcbb2dbc..f1b60716a2ee945 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3923,6 +3923,10 @@ class Sema final {
                                               APValue &Value, CCEKind CCE,
                                               NamedDecl *Dest = nullptr);
 
+  ExprResult EvaluateConvertedConstantExpression(Expr *E, QualType T,
+                                                 APValue &Value, CCEKind CCE,
+                                                 bool RequireInt);
+
   /// Abstract base class used to perform a contextual implicit
   /// conversion from an expression to any type passing a filter.
   class ContextualImplicitConverter {
diff --git a/clang/lib/Parse/ParseTemplate.cpp b/clang/lib/Parse/ParseTemplate.cpp
index f556d0e6d4f8b6e..64fe4d50bba27bf 100644
--- a/clang/lib/Parse/ParseTemplate.cpp
+++ b/clang/lib/Parse/ParseTemplate.cpp
@@ -1062,8 +1062,7 @@ Parser::ParseNonTypeTemplateParameter(unsigned Depth, unsigned Position) {
       ++CurTemplateDepthTracker;
       EnterExpressionEvaluationContext ConstantEvaluated(
           Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
-      DefaultArg =
-          Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression());
+      DefaultArg = Actions.CorrectDelayedTyposInExpr(ParseInitializer());
       if (DefaultArg.isInvalid())
         SkipUntil(tok::comma, tok::greater, StopAtSemi | StopBeforeMatch);
     }
@@ -1582,6 +1581,8 @@ ParsedTemplateArgument Parser::ParseTemplateTemplateArgument() {
 ///         constant-expression
 ///         type-id
 ///         id-expression
+///         braced-init-list  [C++26, DR]
+///
 ParsedTemplateArgument Parser::ParseTemplateArgument() {
   // C++ [temp.arg]p2:
   //   In a template-argument, an ambiguity between a type-id and an
@@ -1619,8 +1620,12 @@ ParsedTemplateArgument Parser::ParseTemplateArgument() {
   }
 
   // Parse a non-type template argument.
+  ExprResult ExprArg;
   SourceLocation Loc = Tok.getLocation();
-  ExprResult ExprArg = ParseConstantExpressionInExprEvalContext(MaybeTypeCast);
+  if (getLangOpts().CPlusPlus11 && Tok.is(tok::l_brace))
+    ExprArg = ParseBraceInitializer();
+  else
+    ExprArg = ParseConstantExpressionInExprEvalContext(MaybeTypeCast);
   if (ExprArg.isInvalid() || !ExprArg.get()) {
     return ParsedTemplateArgument();
   }
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 64607e28b8b35e6..b4a324b9863fa18 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -6206,6 +6206,15 @@ ExprResult Sema::CheckConvertedConstantExpression(Expr *From, QualType T,
   return R;
 }
 
+ExprResult Sema::EvaluateConvertedConstantExpression(Expr *E, QualType T,
+                                                     APValue &Value,
+                                                     Sema::CCEKind CCE,
+                                                     bool RequireInt) {
+
+  APValue PreNarrowingValue;
+  return ::EvaluateConvertedConstantExpression(*this, E, T, Value, CCE,
+                                               RequireInt, PreNarrowingValue);
+}
 
 /// dropPointerConversions - If the given standard conversion sequence
 /// involves any pointer conversions, remove them.  This may change
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index c188dd34014a4b3..b3ab9eeb33ad555 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -7259,49 +7259,73 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
     return E;
   }
 
+  QualType CanonParamType = Context.getCanonicalType(ParamType);
+  // Avoid making a copy when initializing a template parameter of class type
+  // from a template parameter object of the same type. This is going beyond
+  // the standard, but is required for soundness: in
+  //   template<A a> struct X { X *p; X<a> *q; };
+  // ... we need p and q to have the same type.
+  //
+  // Similarly, don't inject a call to a copy constructor when initializing
+  // from a template parameter of the same type.
+  Expr *InnerArg = Arg->IgnoreParenImpCasts();
+  if (ParamType->isRecordType() && isa<DeclRefExpr>(InnerArg) &&
+      Context.hasSameUnqualifiedType(ParamType, InnerArg->getType())) {
+    NamedDecl *ND = cast<DeclRefExpr>(InnerArg)->getDecl();
+    if (auto *TPO = dyn_cast<TemplateParamObjectDecl>(ND)) {
+
+      SugaredConverted = TemplateArgument(TPO, ParamType);
+      CanonicalConverted =
+          TemplateArgument(TPO->getCanonicalDecl(), CanonParamType);
+      return Arg;
+    }
+    if (isa<NonTypeTemplateParmDecl>(ND)) {
+      SugaredConverted = TemplateArgument(Arg);
+      CanonicalConverted =
+          Context.getCanonicalTemplateArgument(SugaredConverted);
+      return Arg;
+    }
+  }
+
   // The initialization of the parameter from the argument is
   // a constant-evaluated context.
   EnterExpressionEvaluationContext ConstantEvaluated(
       *this, Sema::ExpressionEvaluationContext::ConstantEvaluated);
 
-  if (getLangOpts().CPlusPlus17) {
-    QualType CanonParamType = Context.getCanonicalType(ParamType);
-
-    // Avoid making a copy when initializing a template parameter of class type
-    // from a template parameter object of the same type. This is going beyond
-    // the standard, but is required for soundness: in
-    //   template<A a> struct X { X *p; X<a> *q; };
-    // ... we need p and q to have the same type.
-    //
-    // Similarly, don't inject a call to a copy constructor when initializing
-    // from a template parameter of the same type.
-    Expr *InnerArg = Arg->IgnoreParenImpCasts();
-    if (ParamType->isRecordType() && isa<DeclRefExpr>(InnerArg) &&
-        Context.hasSameUnqualifiedType(ParamType, InnerArg->getType())) {
-      NamedDecl *ND = cast<DeclRefExpr>(InnerArg)->getDecl();
-      if (auto *TPO = dyn_cast<TemplateParamObjectDecl>(ND)) {
-
-        SugaredConverted = TemplateArgument(TPO, ParamType);
-        CanonicalConverted =
-            TemplateArgument(TPO->getCanonicalDecl(), CanonParamType);
-        return Arg;
-      }
-      if (isa<NonTypeTemplateParmDecl>(ND)) {
-        SugaredConverted = TemplateArgument(Arg);
-        CanonicalConverted =
-            Context.getCanonicalTemplateArgument(SugaredConverted);
-        return Arg;
-      }
-    }
+  bool IsConvertedConstantExpression = true;
+  if (isa<InitListExpr>(Arg) || ParamType->isRecordType()) {
+    InitializationKind Kind = InitializationKind::CreateForInit(
+        Arg->getBeginLoc(), /*DirectInit*/ false, Arg);
+    Expr *Inits[1] = {Arg};
+    InitializedEntity Entity =
+        InitializedEntity::InitializeTemplateParameter(ParamType, Param);
+    InitializationSequence InitSeq(*this, Entity, Kind, Inits);
+    ExprResult Result = InitSeq.Perform(*this, Entity, Kind, Inits);
+    if (Result.isInvalid() || !Result.get())
+      return ExprError();
+    Result = ActOnConstantExpression(Result.get());
+    if (Result.isInvalid() || !Result.get())
+      return ExprError();
+    Arg = ActOnFinishFullExpr(Result.get(), Arg->getBeginLoc(), false,
+                              /*IsConstexpr=*/true, /*IsTemplateArgument=*/true)
+              .get();
+    IsConvertedConstantExpression = false;
+  }
 
+  if (getLangOpts().CPlusPlus17) {
     // C++17 [temp.arg.nontype]p1:
     //   A template-argument for a non-type template parameter shall be
     //   a converted constant expression of the type of the template-parameter.
     APValue Value;
-    ExprResult ArgResult = CheckConvertedConstantExpression(
-        Arg, ParamType, Value, CCEK_TemplateArg, Param);
-    if (ArgResult.isInvalid())
-      return ExprError();
+    ExprResult ArgResult;
+    if (IsConvertedConstantExpression) {
+      ArgResult = BuildConvertedConstantExpression(Arg, ParamType,
+                                                   CCEK_TemplateArg, Param);
+      if (ArgResult.isInvalid())
+        return ExprError();
+    } else {
+      ArgResult = Arg;
+    }
 
     // For a value-dependent argument, CheckConvertedConstantExpression is
     // permitted (and expected) to be unable to determine a value.
@@ -7312,6 +7336,12 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
       return ArgResult;
     }
 
+    ArgResult = EvaluateConvertedConstantExpression(
+        ArgResult.get(), ParamType, Value, CCEK_TemplateArg, /*RequireInt=*/
+        false);
+    if (ArgResult.isInvalid())
+      return ExprError();
+
     // Convert the APValue to a TemplateArgument.
     switch (Value.getKind()) {
     case APValue::None:
diff --git a/clang/test/CXX/drs/dr20xx.cpp b/clang/test/CXX/drs/dr20xx.cpp
index dd60af14bb6b71d..7b9a5fe0b8076b0 100644
--- a/clang/test/CXX/drs/dr20xx.cpp
+++ b/clang/test/CXX/drs/dr20xx.cpp
@@ -61,6 +61,14 @@ namespace dr2026 { // dr2026: 11
   }
 }
 
+namespace dr2049 { // dr2049: 18 drafting
+#if __cplusplus > 202002L
+template <int* x = {}> struct X {};
+X<> a;
+X<nullptr> b;
+#endif
+}
+
 namespace dr2061 { // dr2061: yes
 #if __cplusplus >= 201103L
   namespace A {
diff --git a/clang/test/CXX/drs/dr24xx.cpp b/clang/test/CXX/drs/dr24xx.cpp
new file mode 100644
index 000000000000000..0e6a90e582c19ec
--- /dev/null
+++ b/clang/test/CXX/drs/dr24xx.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -std=c++98 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors \
+// RUN:            -Wno-variadic-macros -Wno-c11-extensions
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
+// RUN: %clang_cc1 -std=c++17 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
+// RUN: %clang_cc1 -std=c++23 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors
+// expected-no-diagnostics
+
+namespace dr2450 { // dr2450: 18 drafting
+#if __cplusplus > 202002L
+struct S {int a;};
+template <S s>
+void f(){}
+
+void test() {
+f<{0}>();
+f<{.a= 0}>();
+}
+
+#endif
+}
+
+namespace dr2459 { // dr2459: 18 drafting
+#if __cplusplus > 202002L
+struct A {
+  constexpr A(float) {}
+};
+template<A> struct X {};
+X<1> x;
+#endif
+}
diff --git a/clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp b/clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
index 9f9aad604d5f119..5e8cba979751d7d 100644
--- a/clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
+++ b/clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
@@ -62,7 +62,7 @@ namespace ClassNTTP {
   template<A a> constexpr int f() { return a.y; }
   static_assert(f<A{1,2}>() == 2);
 
-  template<A a> int id;
+  template<A a> int id; // expected-note {{passing argument to parameter 'a' here}}
   constexpr A a = {1, 2};
   static_assert(&id<A{1,2}> == &id<a>);
   static_assert(&id<A{1,3}> != &id<a>);
@@ -90,8 +90,8 @@ namespace ConvertedConstant {
     constexpr A(float) {}
   };
   template <A> struct X {};
-  void f(X<1.0f>) {} // OK, user-defined conversion
-  void f(X<2>) {} // expected-error {{conversion from 'int' to 'A' is not allowed in a converted constant expression}}
+  void f(X<1.0f>) {}
+  void g(X<2>) {}
 }
 
 namespace CopyCounting {
diff --git a/clang/test/SemaTemplate/temp_arg_nontype_cxx2c.cpp b/clang/test/SemaTemplate/temp_arg_nontype_cxx2c.cpp
new file mode 100644
index 000000000000000..3ca948f6ca3a666
--- /dev/null
+++ b/clang/test/SemaTemplate/temp_arg_nontype_cxx2c.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -Wconversion -verify %s
+
+struct Test {
+    int a = 0;
+    int b = 42;
+};
+
+template <Test t>
+struct A {
+    static constexpr auto a = t.a;
+    static constexpr auto b = t.b;
+};
+
+template <auto N>
+struct Auto {};
+
+template <typename T, T elem>
+struct Explicit{};
+
+struct L {};
+struct M {};
+
+struct Constructor {
+    Constructor(L) {}; // expected-note {{here}}
+    constexpr Constructor(M){};
+};
+
+template < Test = {} >
+struct DefaultParam1{};
+
+template < Test = {1, 2} >
+struct DefaultParam2{};
+
+template < Test = {. b = 5} >
+struct DefaultParam3{};
+
+void test() {
+    static_assert(A<{}>::a == 0);
+    static_assert(A<{}>::b == 42);
+    static_assert(A<{.a = 3}>::a == 3);
+    static_assert(A<{.b = 4}>::b == 4);
+
+    Auto<{0}> a; // expected-error {{cannot deduce type of initializer list}}
+
+    int notconst = 0; // expected-note {{declared here}}
+    A<{notconst}> _; // expected-error {{non-type template argument is not a constant expression}} \
+                     // expected-note  {{read of non-const variable 'notconst' is not allowed in a constant expression}}
+
+
+    Explicit<Constructor, {L{}}> err; // expected-error {{non-type template argument is not a constant expression}} \
+                                      // expected-note {{non-constexpr constructor 'Constructor' cannot be used in a constant expression}}
+    Explicit<Constructor, {M{}}> ok;
+
+
+    DefaultParam1<> d1;
+    DefaultParam2<> d2;
+    DefaultParam3<> d3;
+}
+
+template<auto n> struct B { /* ... */ };
+template<int i> struct C { /* ... */ };
+C<{ 42 }> c1;  // expected-warning {{braces around scalar initializer}}
+
+struct J1 {
+  J1 *self=this;
+};
+B<J1{}> j1;  // expected-error {{pointer to temporary object is not allowed in a template argument}}
+
+struct J2 {
+  J2 *self=this;
+  constexpr J2() {}
+  constexpr J2(const J2&) {}
+};
+B<J2{}> j2;  // expected-error {{pointer to temporary object is not allowed in a template argument}}
diff --git a/clang/www/cxx_dr_status.html b/clang/www/cxx_dr_status.html
index e711b2173d191c1..f4c508f458c1ab0 100755
--- a/clang/www/cxx_dr_status.html
+++ b/clang/www/cxx_dr_status.html
@@ -12101,7 +12101,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
     <td><a href="https://cplusplus.github.io/CWG/issues/2049.html">2049</a></td>
     <td>drafting</td>
     <td>List initializer in non-type template default argument</td>
-    <td align="center">Not resolved</td>
+    <td class="unreleased" align="center">Clang 18</td>
   </tr>
   <tr id="2050">
     <td><a href="https://cplusplus.github.io/CWG/issues/2050.html">2050</a></td>
@@ -14507,7 +14507,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
     <td><a href="https://cplusplus.github.io/CWG/issues/2450.html">2450</a></td>
     <td>drafting</td>
     <td><I>braced-init-list</I> as a <I>template-argument</I></td>
-    <td align="center">Not resolved</td>
+    <td class="unreleased" align="center">Clang 18</td>
   </tr>
   <tr id="2451">
     <td><a href="https://cplusplus.github.io/CWG/issues/2451.html">2451</a></td>
@@ -14561,7 +14561,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
     <td><a href="https://cplusplus.github.io/CWG/issues/2459.html">2459</a></td>
     <td>drafting</td>
     <td>Template parameter initialization</td>
-    <td align="center">Not resolved</td>
+    <td class="unreleased" align="center">Clang 18</td>
   </tr>
   <tr id="2460">
     <td><a href="https://cplusplus.github.io/CWG/issues/2460.html">2460</a></td>
diff --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html
index 621439d0bae9666..78ac341e4507c70 100755
--- a/clang/www/cxx_status.html
+++ b/clang/www/cxx_status.html
@@ -151,7 +151,7 @@ <h2 id="cxx26">C++2c implementation status</h2>
  <tr>
   <td>Template parameter initialization</td>
   <td><a href="https://wg21.link/P2308R1">P2308R1</a> (<a href="#dr">DR</a>)</td>
-  <td class="none" align="center">No</td>
+  <td class="unreleased" align="center">Clang 18</td>
  </tr>
  <tr>
   <td>Pack Indexing</td>

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

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

DR test side looks good, except for a small nit.

@@ -0,0 +1,32 @@
// RUN: %clang_cc1 -std=c++98 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is triple necessary for the test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need -fexceptions -fcxx-exceptions -Wno-variadic-macros -Wno-c11-extensions either.

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

I dont think the triple is necessary for the test, otherwise this LGTM!

@@ -0,0 +1,32 @@
// RUN: %clang_cc1 -std=c++98 -triple x86_64-unknown-unknown %s -verify -fexceptions -fcxx-exceptions -pedantic-errors \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need -fexceptions -fcxx-exceptions -Wno-variadic-macros -Wno-c11-extensions either.

@@ -62,7 +62,7 @@ namespace ClassNTTP {
template<A a> constexpr int f() { return a.y; }
static_assert(f<A{1,2}>() == 2);

template<A a> int id;
template<A a> int id; // expected-note {{passing argument to parameter 'a' here}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What diagnostic is this note associated with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

L70. you want me to use a label i guess?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh, it was just out of sight. Yeah might as well switch to a bookmark instead. I don't feel that strongly though.

Copy link
Contributor

@tahonermann tahonermann left a comment

Choose a reason for hiding this comment

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

Looks like good work to me!

Comment on lines +67 to +68
X<> a;
X<nullptr> b;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a same-type static assertion for a and b to ensure a consistent specialization is selected?

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks

Result = ActOnConstantExpression(Result.get());
if (Result.isInvalid() || !Result.get())
return ExprError();
Arg = ActOnFinishFullExpr(Result.get(), Arg->getBeginLoc(), false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Arg = ActOnFinishFullExpr(Result.get(), Arg->getBeginLoc(), false,
Arg = ActOnFinishFullExpr(Result.get(), Arg->getBeginLoc(), /*DiscardedValue=*/false,

bool IsConvertedConstantExpression = true;
if (isa<InitListExpr>(Arg) || ParamType->isRecordType()) {
InitializationKind Kind = InitializationKind::CreateForInit(
Arg->getBeginLoc(), /*DirectInit*/ false, Arg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Arg->getBeginLoc(), /*DirectInit*/ false, Arg);
Arg->getBeginLoc(), /*DirectInit=*/false, Arg);

Sema::CCEKind CCE,
bool RequireInt) {

APValue PreNarrowingValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels a bit gratuitous to create a wrapper just to avoid a temporary variable. Maybe if we used it several times but I just see one use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because I wanted to use the same pattern as
CheckConvertedConstantExpression/BuildConvertedConstantExpression which both defer to a static function.
They probably could all be changed to be members of Sema if we wanted to.
I'd rather not do that in this patch though

Copy link
Contributor Author

@cor3ntin cor3ntin Nov 30, 2023

Choose a reason for hiding this comment

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

After offline discussions, i changed it. Let me know if that makes you happier.

@cor3ntin cor3ntin force-pushed the corentin/p2308_2 branch 2 times, most recently from 969629b to eff33d0 Compare November 29, 2023 22:19
Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

LGTM

@cor3ntin cor3ntin merged commit f40d251 into llvm:main Dec 1, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ c++26 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
7 participants