[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.


================
Comment at: clang/include/clang/AST/DeclBase.h:1686
 
-    /// [C++17] Only used by CXXDeductionGuideDecl. Indicates that
-    /// the Deduction Guide is the implicitly generated 'copy
-    /// deduction candidate' (is used during overload resolution).
-    uint64_t IsCopyDeductionCandidate : 1;
+    /// Only used by CXXDeductionGuideDecl. Indicates the kind
+    /// of the Deduction Guide that is the implicitly generated
----------------
Why remove `[C++17]` ?


================
Comment at: clang/include/clang/AST/DeclCXX.h:1987
+  void setDeductionCandidateKind(DeductionCandidateKind K) {
+    FunctionDeclBits.DeductionCandidateKind = static_cast<uint64_t>(K);
   }
----------------
aaron.ballman wrote:
> Er, seems a bit odd to cast an 8-bit type to a 64-bit type only to shove it into a 2-bit bit-field. I think `DeductionCandidateKind` should be an enum class whose underlying type is `int` and we cast to/from `int` as needed.
It feels a bit weird that we are taking an enum w/ an underlying type of `unsigned char` casting it to `int` and then placing it in an unsigned bit-field. I don't have a better suggestion ATM but I wish we had something better. 


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12647
     // FIXME: Initialization should not be taking a mutable list of inits.
-    SmallVector<Expr*, 8> InitsCopy(DeduceInits.begin(), DeduceInits.end());
+    SmallVector<Expr *, 8> InitsCopy(DeduceInits.begin(), DeduceInits.end());
     return DeduceTemplateSpecializationFromInitializer(TSI, Entity, Kind,
----------------
nitpick but we seem to use `Expr*` everywhere else.


================
Comment at: clang/lib/Sema/SemaInit.cpp:15
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
----------------
I saw your adding headers. How did you figure out which ones were missing?


================
Comment at: clang/lib/Sema/SemaInit.cpp:513
+                  SmallVectorImpl<QualType> &AggrDeductionCandidateParamTypes)
+      : InitListChecker(S, Entity, IL, T, true, false, false,
+                        &AggrDeductionCandidateParamTypes){};
----------------
nit


================
Comment at: clang/lib/Sema/SemaInit.cpp:10680
+                                   /*PartialOverloading=*/false, AllowExplicit,
+                                   ADLCallKind::NotADL, {}, true);
+    } else {
----------------
nit,

note: PO` is not a very descriptive name.


================
Comment at: clang/lib/Sema/SemaInit.cpp:10736
+            addDeductionCandidate(TD, GD, DeclAccessPair::make(TD, AS_public),
+                                  OnlyListConstructors, true);
+          }
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:10743
+          addDeductionCandidate(TD, GD, DeclAccessPair::make(TD, AS_public),
+                                OnlyListConstructors, true);
+        }
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:10763
 
-      // C++ [over.match.ctor]p1: (non-list copy-initialization from non-class)
-      //   For copy-initialization, the candidate functions are all the
-      //   converting constructors (12.3.1) of that class.
-      // C++ [over.match.copy]p1: (non-list copy-initialization from class)
-      //   The converting constructors of T are candidate functions.
-      if (!AllowExplicit) {
-        // Overload resolution checks whether the deduction guide is declared
-        // explicit for us.
-
-        // When looking for a converting constructor, deduction guides that
-        // could never be called with one argument are not interesting to
-        // check or note.
-        if (GD->getMinRequiredArguments() > 1 ||
-            (GD->getNumParams() == 0 && !GD->isVariadic()))
-          continue;
+      addDeductionCandidate(TD, GD, I.getPair(), OnlyListConstructors, false);
+    }
----------------
> Quoted Text




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139837/new/

https://reviews.llvm.org/D139837