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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
