[PATCH] D12686: Add support for GCC's '__auto_type' extension.

[Richard Smith via cfe-commits]

rsmith added a comment.

Sorry for the long delay. This looks to be in good shape.


================
Comment at: include/clang/AST/Type.h:1210
@@ +1209,3 @@
+/// Which keyword(s) were used to create an AutoType.
+enum class AutoTypeKeyword : unsigned char {
+  /// \brief auto
----------------
The explicit underlying type here doesn't seem necessary.

================
Comment at: include/clang/AST/Type.h:1438
@@ -1429,1 +1437,3 @@
+    /// Was this placeholder type spelled as 'auto', 'decltype(auto)', or '__auto_type'?
+    AutoTypeKeyword Keyword : 2;
   };
----------------
Use `unsigned` not `AutoTypeKeyword`. MSVC struct layout inserts extra padding if your bit-fields don't all have the same type.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1678
@@ -1674,3 +1677,3 @@
   "|in template argument|in typedef|in type alias|in function return type"
-  "|in conversion function type|here|in lambda parameter}1">;
+  "|in conversion function type|here|in lambda parameter|in 'new' argument}1">;
 def err_auto_not_allowed_var_inst : Error<
----------------
How about "in type allocated by `new`" or similar? The type operand isn't really an argument.

================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:4007-4012
@@ -4006,4 +4006,8 @@
 
   InitListExpr *InitList = dyn_cast<InitListExpr>(Init);
   if (InitList) {
+    if (!getLangOpts().CPlusPlus) {
+      Diag(Loc, diag::err_auto_init_list_from_c);
+      return DAR_Failed;
+    }
     for (unsigned i = 0, e = InitList->getNumInits(); i < e; ++i) {
----------------
Move this bailout to before we build the template parameter and substitute it into the type.

================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:4011
@@ +4010,3 @@
+      Diag(Loc, diag::err_auto_init_list_from_c);
+      return DAR_Failed;
+    }
----------------
Should be `DAR_FailedAlreadyDiagnosed` so the caller doesn't diagnose it again.

================
Comment at: lib/Sema/SemaType.cpp:1481
@@ -1479,3 +1480,3 @@
     // being analyzed (which tracks the invented type template parameter).
     if (declarator.getContext() == Declarator::LambdaExprParameterContext) {
       sema::LambdaScopeInfo *LSI = S.getCurLambda();
----------------
Can you move the `auto_type` case label lower to make this more obvious? (Put a `break` into the `if` and replace the `else` with fallthrough to avoid putting the label inside the `else`.)

================
Comment at: lib/Sema/SemaType.cpp:1517
@@ +1516,3 @@
+    Result = Context.getAutoType(QualType(),
+                                 /*Keyword*/     AutoTypeKeyword::DecltypeAuto,
+                                 /*IsDependent*/ false);
----------------
The `/*Keyword*/` comment here doesn't add anything, thanks to your enumeration. Remove it and clang-format this line?

================
Comment at: lib/Sema/SemaType.cpp:2621
@@ -2620,3 +2620,3 @@
   // C++14 In generic lambdas allow 'auto' in their parameters.
-  if (ContainsPlaceholderType &&
+  if (D.getDeclSpec().containsPlaceholderType() &&
       (!SemaRef.getLangOpts().CPlusPlus11 || !D.isFunctionDeclarator())) {
----------------
Does this still do the right thing for the `IK_ConversionFunctionId` case? It doesn't look like it will: in that case, the `auto` is in the //declarator-id//, not in the //decl-specifier-seq//.

================
Comment at: lib/Sema/SemaType.cpp:2626-2627
@@ -2625,4 +2625,4 @@
     switch (D.getContext()) {
     case Declarator::KNRTypeListContext:
       llvm_unreachable("K&R type lists aren't allowed in C++");
     case Declarator::LambdaExprContext:
----------------
This is now reachable.

================
Comment at: lib/Sema/SemaType.cpp:3756-3760
@@ -3743,2 +3755,7 @@
           }
+        } else if (D.getDeclSpec().getTypeSpecType() == DeclSpec::TST_auto_type) {
+          S.Diag(D.getDeclSpec().getTypeSpecTypeLoc(),
+                 diag::err_auto_not_allowed)
+            << 2 << 13 << D.getDeclSpec().getSourceRange();
+          D.setInvalidType(true);
         }
----------------
Please deal with this in the "checking for auto" block at the top rather than repeating the diagnostic code here with magic numbers; maybe change

  (!SemaRef.getLangOpts().CPlusPlus11 || !D.isFunctionDeclarator())) {

to

  (!SemaRef.getLangOpts().CPlusPlus11 || !D.isFunctionDeclarator() || D.getDeclSpec().getTypeSpecType() == DeclSpec::TST_auto_type)) {

up there.


http://reviews.llvm.org/D12686