[Clang] Add __builtin_common_reference

[philnik777]

No description provided.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

---

Patch is 40.04 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/121199.diff

9 Files Affected:

• (modified) clang/docs/LanguageExtensions.rst (+17)
• (modified) clang/include/clang/Basic/BuiltinTemplates.td (+28-2)
• (modified) clang/include/clang/Sema/Sema.h (+19)
• (modified) clang/lib/Sema/SemaExprCXX.cpp (+5-87)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+335-18)
• (modified) clang/lib/Sema/SemaType.cpp (+75)
• (added) clang/test/SemaCXX/type-trait-common-reference.cpp (+141)
• (modified) libcxx/include/__type_traits/common_reference.h (+26-11)
• (modified) libcxx/include/module.modulemap (+1)

diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst
index 3b8a9cac6587a..6fc86507ca423 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -1677,6 +1677,23 @@ Builtin type aliases
 
 Clang provides a few builtin aliases to improve the throughput of certain metaprogramming facilities.
 
+__builtin_common_reference
+--------------------------
+
+.. code-block:: c++
+
+  template <template <class, class, template <class> class, template <class> class> class BasicCommonReferenceT,
+            template <class... Args> CommonTypeT,
+            template <class> HasTypeMember,
+            class HasNoTypeMember,
+            class... Ts>
+  using __builtin_common_reference = ...;
+
+This alias is used for implementing ``std::common_refernce``. If ``std::common_reference`` should contain a ``type``
+member, it is an alias to ``HasTypeMember<TheCommonReference>``. Otherwse it is an alias to ``HasNoTypeMember``. The
+``CommonTypeT`` is usually ``std::common_type_t``. ``BasicCommonReferenceT`` is usually an alias template to
+``basic_common_reference<T, U, TX, UX>::type``.
+
 __builtin_common_type
 ---------------------
 
diff --git a/clang/include/clang/Basic/BuiltinTemplates.td b/clang/include/clang/Basic/BuiltinTemplates.td
index d46ce063d2f7e..5c79e89800829 100644
--- a/clang/include/clang/Basic/BuiltinTemplates.td
+++ b/clang/include/clang/Basic/BuiltinTemplates.td
@@ -10,11 +10,11 @@ class TemplateArg<string name> {
   string Name = name;
 }
 
-class Template<list<TemplateArg> args, string name> : TemplateArg<name> {
+class Template<list<TemplateArg> args, string name = ""> : TemplateArg<name> {
   list<TemplateArg> Args = args;
 }
 
-class Class<string name, bit is_variadic = 0> : TemplateArg<name> {
+class Class<string name = "", bit is_variadic = 0> : TemplateArg<name> {
   bit IsVariadic = is_variadic;
 }
 
@@ -50,3 +50,29 @@ def __builtin_common_type : BuiltinTemplate<
    Template<[Class<"TypeMember">], "HasTypeMember">,
    Class<"HasNoTypeMember">,
    Class<"Ts", /*is_variadic=*/1>]>;
+
+// template <template <class,"
+//                     class,"
+//                     template <class> class,"
+//                     template <class> class> class BasicCommonReferenceT,"
+//           template <class... Args> class CommonTypeT,"
+//           template <class> class HasTypeMember,"
+//           class HasNoTypeMember,"
+//           class... Ts>"
+def __builtin_common_reference : BuiltinTemplate<
+            [Template<[Class<>,
+                       Class<>,
+                       Template<[Class<>]>,
+                       Template<[Class<>]>], "BasicCommonReferenceT">,
+             Template<[Class<"Args", /*is_variadic=*/1>], "CommonTypeT">,
+             Template<[Class<>], "HasTypeMember">,
+             Class<"HasNoTypeMember">,
+             Class<"Ts", /*is_variadic=*/1>]>;
+
+foreach Ref = ["", "lvalue", "rvalue"] in {
+  foreach Const = ["", "const"] in {
+    foreach Volatile = ["", "volatile"] in {
+      def __clang_internal_xref_#Ref#Const#Volatile : BuiltinTemplate<[Class<>]>;
+    }
+  }
+}
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 066bce61c74c1..762cb851ee04f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -15076,15 +15076,34 @@ class Sema final : public SemaBase {
   QualType BuiltinDecay(QualType BaseType, SourceLocation Loc);
   QualType BuiltinAddReference(QualType BaseType, UTTKind UKind,
                                SourceLocation Loc);
+
+  QualType BuiltinAddRValueReference(QualType BaseType, SourceLocation Loc) {
+    return BuiltinAddReference(BaseType, UnaryTransformType::AddRvalueReference,
+                               Loc);
+  }
+
+  QualType BuiltinAddLValueReference(QualType BaseType, SourceLocation Loc) {
+    return BuiltinAddReference(BaseType, UnaryTransformType::AddLvalueReference,
+                               Loc);
+  }
+
   QualType BuiltinRemoveExtent(QualType BaseType, UTTKind UKind,
                                SourceLocation Loc);
   QualType BuiltinRemoveReference(QualType BaseType, UTTKind UKind,
                                   SourceLocation Loc);
+
+  QualType BuiltinRemoveCVRef(QualType BaseType, SourceLocation Loc) {
+    return BuiltinRemoveReference(BaseType, UTTKind::RemoveCVRef, Loc);
+  }
+
   QualType BuiltinChangeCVRQualifiers(QualType BaseType, UTTKind UKind,
                                       SourceLocation Loc);
   QualType BuiltinChangeSignedness(QualType BaseType, UTTKind UKind,
                                    SourceLocation Loc);
 
+  bool BuiltinIsConvertible(QualType From, QualType To, SourceLocation Loc,
+                            bool CheckNothrow = false);
+
   /// Ensure that the type T is a literal type.
   ///
   /// This routine checks whether the type @p T is a literal type. If @p T is an
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 46895db4a0756..5ca5a8a57fa0f 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5739,76 +5739,6 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
 static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceInfo *Lhs,
                                     const TypeSourceInfo *Rhs, SourceLocation KeyLoc);
 
-static ExprResult CheckConvertibilityForTypeTraits(
-    Sema &Self, const TypeSourceInfo *Lhs, const TypeSourceInfo *Rhs,
-    SourceLocation KeyLoc, llvm::BumpPtrAllocator &OpaqueExprAllocator) {
-
-  QualType LhsT = Lhs->getType();
-  QualType RhsT = Rhs->getType();
-
-  // C++0x [meta.rel]p4:
-  //   Given the following function prototype:
-  //
-  //     template <class T>
-  //       typename add_rvalue_reference<T>::type create();
-  //
-  //   the predicate condition for a template specialization
-  //   is_convertible<From, To> shall be satisfied if and only if
-  //   the return expression in the following code would be
-  //   well-formed, including any implicit conversions to the return
-  //   type of the function:
-  //
-  //     To test() {
-  //       return create<From>();
-  //     }
-  //
-  //   Access checking is performed as if in a context unrelated to To and
-  //   From. Only the validity of the immediate context of the expression
-  //   of the return-statement (including conversions to the return type)
-  //   is considered.
-  //
-  // We model the initialization as a copy-initialization of a temporary
-  // of the appropriate type, which for this expression is identical to the
-  // return statement (since NRVO doesn't apply).
-
-  // Functions aren't allowed to return function or array types.
-  if (RhsT->isFunctionType() || RhsT->isArrayType())
-    return ExprError();
-
-  // A function definition requires a complete, non-abstract return type.
-  if (!Self.isCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT) ||
-      Self.isAbstractType(Rhs->getTypeLoc().getBeginLoc(), RhsT))
-    return ExprError();
-
-  // Compute the result of add_rvalue_reference.
-  if (LhsT->isObjectType() || LhsT->isFunctionType())
-    LhsT = Self.Context.getRValueReferenceType(LhsT);
-
-  // Build a fake source and destination for initialization.
-  InitializedEntity To(InitializedEntity::InitializeTemporary(RhsT));
-  Expr *From = new (OpaqueExprAllocator.Allocate<OpaqueValueExpr>())
-      OpaqueValueExpr(KeyLoc, LhsT.getNonLValueExprType(Self.Context),
-                      Expr::getValueKindForType(LhsT));
-  InitializationKind Kind =
-      InitializationKind::CreateCopy(KeyLoc, SourceLocation());
-
-  // Perform the initialization in an unevaluated context within a SFINAE
-  // trap at translation unit scope.
-  EnterExpressionEvaluationContext Unevaluated(
-      Self, Sema::ExpressionEvaluationContext::Unevaluated);
-  Sema::SFINAETrap SFINAE(Self, /*AccessCheckingSFINAE=*/true);
-  Sema::ContextRAII TUContext(Self, Self.Context.getTranslationUnitDecl());
-  InitializationSequence Init(Self, To, Kind, From);
-  if (Init.Failed())
-    return ExprError();
-
-  ExprResult Result = Init.Perform(Self, To, Kind, From);
-  if (Result.isInvalid() || SFINAE.hasErrorOccurred())
-    return ExprError();
-
-  return Result;
-}
-
 static APValue EvaluateSizeTTypeTrait(Sema &S, TypeTrait Kind,
                                       SourceLocation KWLoc,
                                       ArrayRef<TypeSourceInfo *> Args,
@@ -5958,9 +5888,8 @@ static bool EvaluateBooleanTypeTrait(Sema &S, TypeTrait Kind,
           S.Context.getPointerType(T.getNonReferenceType()));
       TypeSourceInfo *UPtr = S.Context.CreateTypeSourceInfo(
           S.Context.getPointerType(U.getNonReferenceType()));
-      return !CheckConvertibilityForTypeTraits(S, UPtr, TPtr, RParenLoc,
-                                               OpaqueExprAllocator)
-                  .isInvalid();
+      return S.BuiltinIsConvertible(UPtr->getType(), TPtr->getType(),
+                                    RParenLoc);
     }
 
     if (Kind == clang::TT_IsNothrowConstructible)
@@ -6200,20 +6129,9 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
   }
   case BTT_IsConvertible:
   case BTT_IsConvertibleTo:
-  case BTT_IsNothrowConvertible: {
-    if (RhsT->isVoidType())
-      return LhsT->isVoidType();
-    llvm::BumpPtrAllocator OpaqueExprAllocator;
-    ExprResult Result = CheckConvertibilityForTypeTraits(Self, Lhs, Rhs, KeyLoc,
-                                                         OpaqueExprAllocator);
-    if (Result.isInvalid())
-      return false;
-
-    if (BTT != BTT_IsNothrowConvertible)
-      return true;
-
-    return Self.canThrow(Result.get()) == CT_Cannot;
-  }
+  case BTT_IsNothrowConvertible:
+    return Self.BuiltinIsConvertible(LhsT, RhsT, KeyLoc,
+                                     BTT == BTT_IsNothrowConvertible);
 
   case BTT_IsAssignable:
   case BTT_IsNothrowAssignable:
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index be81b6a46b2c0..47601c1840b92 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -3097,6 +3097,33 @@ void Sema::NoteAllFoundTemplates(TemplateName Name) {
   }
 }
 
+static QualType InstantiateTemplate(Sema &S, TemplateName Template,
+                                    ArrayRef<TemplateArgument> Args,
+                                    SourceLocation Loc) {
+  TemplateArgumentListInfo ArgList;
+  for (auto Arg : Args) {
+    if (Arg.getKind() == TemplateArgument::Type) {
+      ArgList.addArgument(TemplateArgumentLoc(
+          Arg, S.Context.getTrivialTypeSourceInfo(Arg.getAsType())));
+    } else {
+      ArgList.addArgument(
+          S.getTrivialTemplateArgumentLoc(Arg, QualType(), Loc));
+    }
+  }
+
+  EnterExpressionEvaluationContext UnevaluatedContext(
+      S, Sema::ExpressionEvaluationContext::Unevaluated);
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+  Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
+
+  QualType Instantiation = S.CheckTemplateIdType(Template, Loc, ArgList);
+
+  if (SFINAE.hasErrorOccurred())
+    return QualType();
+
+  return Instantiation;
+}
+
 static QualType builtinCommonTypeImpl(Sema &S, TemplateName BaseTemplate,
                                       SourceLocation TemplateLoc,
                                       ArrayRef<TemplateArgument> Ts) {
@@ -3107,24 +3134,7 @@ static QualType builtinCommonTypeImpl(Sema &S, TemplateName BaseTemplate,
     if (T1.getAsType()->isBuiltinType() && T2.getAsType()->isBuiltinType())
       return builtinCommonTypeImpl(S, BaseTemplate, TemplateLoc, {T1, T2});
 
-    TemplateArgumentListInfo Args;
-    Args.addArgument(TemplateArgumentLoc(
-        T1, S.Context.getTrivialTypeSourceInfo(T1.getAsType())));
-    Args.addArgument(TemplateArgumentLoc(
-        T2, S.Context.getTrivialTypeSourceInfo(T2.getAsType())));
-
-    EnterExpressionEvaluationContext UnevaluatedContext(
-        S, Sema::ExpressionEvaluationContext::Unevaluated);
-    Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
-    Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
-
-    QualType BaseTemplateInst =
-        S.CheckTemplateIdType(BaseTemplate, TemplateLoc, Args);
-
-    if (SFINAE.hasErrorOccurred())
-      return QualType();
-
-    return BaseTemplateInst;
+    return InstantiateTemplate(S, BaseTemplate, {T1, T2}, TemplateLoc);
   };
 
   // Note A: For the common_type trait applied to a template parameter pack T of
@@ -3231,6 +3241,230 @@ static QualType builtinCommonTypeImpl(Sema &S, TemplateName BaseTemplate,
   }
 }
 
+static QualType CopyCV(QualType From, QualType To) {
+  if (From.isConstQualified())
+    To.addConst();
+  if (From.isVolatileQualified())
+    To.addVolatile();
+  return To;
+}
+
+// COND-RES(X, Y) be decltype(false ? declval<X(&)()>()() : declval<Y(&)()>()())
+static QualType CondRes(Sema &S, QualType X, QualType Y, SourceLocation Loc) {
+  EnterExpressionEvaluationContext UnevaluatedContext(
+      S, Sema::ExpressionEvaluationContext::Unevaluated);
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+  Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
+
+  // false
+  OpaqueValueExpr CondExpr(SourceLocation(), S.Context.BoolTy, VK_PRValue);
+  ExprResult Cond = &CondExpr;
+
+  // declval<X(&)()>()()
+  OpaqueValueExpr LHSExpr(Loc, X.getNonLValueExprType(S.Context),
+                          Expr::getValueKindForType(X));
+  ExprResult LHS = &LHSExpr;
+
+  // declval<Y(&)()>()()
+  OpaqueValueExpr RHSExpr(Loc, Y.getNonLValueExprType(S.Context),
+                          Expr::getValueKindForType(Y));
+  ExprResult RHS = &RHSExpr;
+
+  ExprValueKind VK = VK_PRValue;
+  ExprObjectKind OK = OK_Ordinary;
+
+  // decltype(false ? declval<X(&)()>()() : declval<Y(&)()>()())
+  QualType Result = S.CheckConditionalOperands(Cond, LHS, RHS, VK, OK, Loc);
+
+  if (SFINAE.hasErrorOccurred())
+    return QualType();
+  if (VK == VK_LValue)
+    return S.BuiltinAddLValueReference(Result, Loc);
+  if (VK == VK_XValue)
+    return S.BuiltinAddRValueReference(Result, Loc);
+  return Result;
+}
+
+static QualType CommonRef(Sema &S, QualType A, QualType B, SourceLocation Loc) {
+  // Given types A and B, let X be remove_reference_t<A>, let Y be
+  // remove_reference_t<B>, and let COMMON-​REF(A, B) be:
+  assert(A->isReferenceType() && B->isReferenceType() &&
+         "A and B have to be ref qualified for a COMMON-REF");
+  auto X = A.getNonReferenceType();
+  auto Y = B.getNonReferenceType();
+
+  // If A and B are both lvalue reference types, COMMON-REF(A, B) is
+  // COND-RES(COPYCV(X, Y) &, COPYCV(​Y, X) &) if that type exists and is a
+  // reference type.
+  if (A->isLValueReferenceType() && B->isLValueReferenceType()) {
+    auto CR = CondRes(S, S.BuiltinAddLValueReference(CopyCV(X, Y), Loc),
+                      S.BuiltinAddLValueReference(CopyCV(Y, X), Loc), Loc);
+    if (CR.isNull() || !CR->isReferenceType())
+      return QualType();
+    return CR;
+  }
+
+  // Otherwise, let C be remove_reference_t<COMMON-REF(X&, Y&)>&&. If A and B
+  // are both rvalue reference types, C is well-formed, and
+  // is_convertible_v<A, C> && is_convertible_v<B, C> is true, then
+  // COMMON-REF(A, B) is C.
+  if (A->isRValueReferenceType() && B->isRValueReferenceType()) {
+    auto C = CommonRef(S, S.BuiltinAddLValueReference(X, Loc),
+                       S.BuiltinAddLValueReference(Y, Loc), Loc);
+    if (C.isNull())
+      return QualType();
+
+    C = C.getNonReferenceType();
+
+    if (S.BuiltinIsConvertible(A, C, Loc) && S.BuiltinIsConvertible(B, C, Loc))
+      return S.BuiltinAddRValueReference(C, Loc);
+    return QualType();
+  }
+
+  // Otherwise, if A is an lvalue reference and B is an rvalue reference, then
+  // COMMON-REF(A, B) is COMMON-REF(B, A).
+  if (A->isLValueReferenceType() && B->isRValueReferenceType())
+    std::swap(A, B);
+
+  // Otherwise, let D be COMMON-REF(const X&, Y&). If A is an rvalue reference
+  // and B is an lvalue reference and D is well-formed and
+  // is_convertible_v<A, D> is true, then COMMON-REF(A, B) is D.
+  if (A->isRValueReferenceType() && B->isLValueReferenceType()) {
+    auto X2 = X;
+    X2.addConst();
+    auto D = CommonRef(S, S.BuiltinAddLValueReference(X2, Loc),
+                       S.BuiltinAddLValueReference(Y, Loc), Loc);
+    if (!D.isNull() && S.BuiltinIsConvertible(A, D, Loc))
+      return D;
+    return QualType();
+  }
+
+  // Otherwise, COMMON-REF(A, B) is ill-formed.
+  // This is implemented by returning from the individual branches above.
+
+  llvm_unreachable("The above cases should be exhaustive");
+}
+
+static QualType builtinCommonReferenceImpl(Sema &S,
+                                           TemplateName CommonReference,
+                                           TemplateName CommonType,
+                                           SourceLocation TemplateLoc,
+                                           ArrayRef<TemplateArgument> Ts) {
+  switch (Ts.size()) {
+  // If sizeof...(T) is zero, there shall be no member type.
+  case 0:
+    return QualType();
+
+  // Otherwise, if sizeof...(T) is one, let T0 denote the sole type in the
+  // pack T. The member typedef type shall denote the same type as T0.
+  case 1:
+    return Ts[0].getAsType();
+
+  // Otherwise, if sizeof...(T) is two, let T1 and T2 denote the two types in
+  // the pack T. Then
+  case 2: {
+    auto T1 = Ts[0].getAsType();
+    auto T2 = Ts[1].getAsType();
+
+    // Let R be COMMON-REF(T1, T2). If T1 and T2 are reference types, R is
+    // well-formed, and is_convertible_v<add_pointer_t<T1>, add_pointer_t<R>> &&
+    // is_convertible_v<add_pointer_t<T2>, add_pointer_t<R>> is true, then the
+    // member typedef type denotes R.
+    if (T1->isReferenceType() && T2->isReferenceType()) {
+      QualType R = CommonRef(S, T1, T2, TemplateLoc);
+      if (!R.isNull()) {
+        if (S.BuiltinIsConvertible(S.BuiltinAddPointer(T1, TemplateLoc),
+                                   S.BuiltinAddPointer(R, TemplateLoc),
+                                   TemplateLoc) &&
+            S.BuiltinIsConvertible(S.BuiltinAddPointer(T2, TemplateLoc),
+                                   S.BuiltinAddPointer(R, TemplateLoc),
+                                   TemplateLoc)) {
+          return R;
+        }
+      }
+    }
+
+    // Otherwise, if basic_common_reference<remove_cvref_t<T1>,
+    // remove_cvref_t<T2>, ​XREF(​T1), XREF(T2)>​::​type is well-formed,
+    // then the member typedef type denotes that type.
+    {
+      auto getXRef = [&](QualType T) {
+        static BuiltinTemplateDecl *Quals[12] = {
+            S.Context.get__clang_internal_xref_Decl(),
+            S.Context.get__clang_internal_xref_constDecl(),
+            S.Context.get__clang_internal_xref_volatileDecl(),
+            S.Context.get__clang_internal_xref_constvolatileDecl(),
+            S.Context.get__clang_internal_xref_lvalueDecl(),
+            S.Context.get__clang_internal_xref_lvalueconstDecl(),
+            S.Context.get__clang_internal_xref_lvaluevolatileDecl(),
+            S.Context.get__clang_internal_xref_lvalueconstvolatileDecl(),
+            S.Context.get__clang_internal_xref_rvalueDecl(),
+            S.Context.get__clang_internal_xref_rvalueconstDecl(),
+            S.Context.get__clang_internal_xref_rvaluevolatileDecl(),
+            S.Context.get__clang_internal_xref_rvalueconstvolatileDecl(),
+        };
+        size_t Index = 0;
+        if (T->isLValueReferenceType()) {
+          T = T.getNonReferenceType();
+          Index += 4;
+        } else if (T->isRValueReferenceType()) {
+          T = T.getNonReferenceType();
+          Index += 8;
+        }
+        if (T.isConstQualified())
+          Index += 1;
+
+        if (T.isVolatileQualified())
+          Index += 2;
+
+        return Quals[Index];
+      };
+
+      auto BCR = InstantiateTemplate(S, CommonReference,
+                                     {S.BuiltinRemoveCVRef(T1, TemplateLoc),
+                                      S.BuiltinRemoveCVRef(T2, TemplateLoc),
+                                      TemplateName{getXRef(T1)},
+                                      TemplateName{getXRef(T2)}},
+                                     TemplateLoc);
+      if (!BCR.isNull())
+        return BCR;
+    }
+
+    // Otherwise, if COND-RES(T1...
[truncated]

[huixie90]

perhaps update the status of P2655 is partially implemented?

[philnik777]

I'd rather just implement the paper separately. It feels like there are a few too many conditions to say "partially implemented".

[huixie90]

sure. but my concern is that your builtin implementation seems to be based on the wording after P2655. However, the original implementation of common_reference.h is pre P2655.

So depending on wether the builtin is available, the behaviour of the library will be different. I would prefer to updating the common_reference.h 's non-builtin branch to match the builtin branch

[philnik777]

I don't see why updating the non-builtin implementation should block implementing a builtin for it. According to the paper it makes almost no difference, so why is it really important here?

[huixie90]

I don't see why updating the non-builtin implementation should block implementing a builtin for it. According to the paper it makes almost no difference, so why is it really important here?

For

common_reference_t<reference_wrapper<int>, int& ,

Your builtin yields int&,

But the non-builtin template implementation gives int.

We will see different behaviours depending on whether or not the builtin is available. Why not just updating the second branch to match the builtin branch?

[huixie90]

Because with this patch, you are effectively implemented the paper, but only partially for some users but not others. And user see different results depending on the compiler version.

[philnik777]

Users see different results based on the compiler version all the time. How is this different?

[huixie90]

Because this is literally coded in the Libc++ code :

if has builtin:
Return int&
Else
Return int

I don’t think we have anything like that in other places

[huixie90]

Updating the else branch to int& is just the natural thing to do

And if you really really want to separate this patch from Libc++ , then just implement the builtin and don’t use it in Libc++

[philnik777]

I want to use the libc++ tests though to make sure it works as expected.

[huixie90]

another issue regarding this is that, this effectively DRed the c++23 paper P2655R3 into previous version (namely C++20). is this intentional ?
my position is to DR it regardless but I want to bring it up for discussion

[frederick-vs-ja]

I think P2655 should be treated as a DR per this SG9 documentation.

[huixie90]

that is just the early stage SG9 discussion on the R0 version. I don't remember the details about DR discussion to be honest

[huixie90]

FWIW, I created patch #141408 to implement the libc++ counter part of the missing bit.
So can i request to merge this patch after landing of my patch? the benifits are :

• Avoid behaviour differences between the two branches
• having more test coverage for the new behaviour that the paper introduces

[frederick-vs-ja]

FYI I've opened #141465 to add _LIBCPP_NO_SPECIALIZATIONS to common_reference. Once the affected test gets fixed, we can also add _LIBCPP_NO_SPECIALIZATIONS here.

[erichkeane]

This seems reasonable to me? I'm not a huge fan of how much is being put into SemaTemplate.cpp to support all the builtins though. I'm wondering if we need to split that up in a follow-up and have a SemaTemplateBuiltins for all of these to live.

Please let Aaron/others take a poke at this, and make sure there are a few more Libc++ reviewers to make sure you're all in agreement with how this works (I see the convo that is happening).

Also, should we loop in libstdc++ folks here?

[erichkeane]

Sorry if I'm being a little dense here... what is going on for the forloop?

[erichkeane]

ah, hrmph.... I see now.

[philnik777]

Yeah, it's not exactly beautiful. I've thought about adding a new AST node for this, but that seems a bit overkill. If anybody has an idea on how to do this more nicely I'm all ears.

[philnik777]

This seems reasonable to me? I'm not a huge fan of how much is being put into SemaTemplate.cpp to support all the builtins though. I'm wondering if we need to split that up in a follow-up and have a SemaTemplateBuiltins for all of these to live.

I can take a look at splitting things up in a follow-up. Shouldn't be too complicated. I do wonder whether it's worth the split, since it's a relatively small amount of code. OTOH it's a very clean split...

Please let Aaron/others take a poke at this, and make sure there are a few more Libc++ reviewers to make sure you're all in agreement with how this works (I see the convo that is happening).

Also, should we loop in libstdc++ folks here?

IDK whether they have any thought on this, but certainly doesn't hurt. CC @jwakely