Don't reject tag declarations in for loop clause-1

[aaron.ballman]

Summary

We currently reject this valid C construct by claiming it declares a non-local variable: for (struct { int i; } s={0}; s.i != 0; s.i--) ;

The problem is that we expect all declaration in the clause-1 declaration statement to be VarDecls of local variables, but there can be other declarations involved such as a tag declaration. We now ignore tag declarations when deciding whether the clause-1 declarations are valid or not. This fixes PR35757.

[aaron.ballman]

Ping

[aaron.ballman]

In D92577#2445714, @aaron.ballman wrote:

Ping

Ping

[rjmccall]

Please test that there's actually an object declared and that it's not *just* a tag declaration.

[aaron.ballman]

Updating based on review feedback.

[aaron.ballman]

In D92577#2458903, @rjmccall wrote:

Please test that there's actually an object declared and that it's not *just* a tag declaration.

Good catch, I've corrected this and added some more test coverage.

[rjmccall]

clang/lib/Sema/SemaStmt.cpp

1848

You can have multiple tag declarations because of complex declarators or type-specifiers, e.g. struct A (*var)(struct B) or _Atomic(struct A(*)(struct B)). It should already be an error to write the latter without a declarator, but you do need to not diagnose if there *is* a declarator. Probably the right way to write this is to ignore tags in the first walk but remember whether you saw a non-tag; if you don't, diagnose the first tag.

[aaron.ballman]

Updating based on review feedback.

clang/lib/Sema/SemaStmt.cpp

1848

Good point on the more complex declarators -- I've added some extra tests for that case as well.

[rjmccall]

LGTM

[aaron.ballman]

Thank you for the review! I've commit in 2d2498ec6c42b12eae873257e6ddce3333fe8348

clang/lib/Sema/SemaStmt.cpp

if (!getLangOpts().CPlusPlus) {
     if (DeclStmt *DS = dyn_cast_or_null<DeclStmt>(First)) {
       // C99 6.8.5p3: The declaration part of a 'for' statement shall only
       // declare identifiers for objects having storage class 'auto' or
       // 'register'.
       for (auto *DI : DS->decls()) {
         VarDecl *VD = dyn_cast<VarDecl>(DI);
         if (VD && VD->isLocalVarDecl() && !VD->hasLocalStorage())
           VD = nullptr;
-        if (!VD) {
+        // It is possible to declare non-variable declarations as part of the
+        // declaration part of a 'for' statement, such as defining a structure
+        // or enum type: for (enum { zero, ten } i = zero; i < ten; ++i);
+        if (!VD && !isa<TagDecl>(DI)) {
           Diag(DI->getLocation(), diag::err_non_local_variable_decl_in_for);
           DI->setInvalidDecl();
         }
       }
     }
   }
 
   CheckBreakContinueBinding(Second.get().second);
   CheckBreakContinueBinding(third.get());
 
   if (!Second.get().first)
     CheckForLoopConditionalStatement(*this, Second.get().second, third.get(),
                                      Body);
   CheckForRedundantIteration(*this, third.get(), Body);
 
   if (Second.get().second &&
       !Diags.isIgnored(diag::warn_comma_operator,
                        Second.get().second->getExprLoc()))
     CommaVisitor(*this).Visit(Second.get().second);
 
   Expr *Third  = third.release().getAs<Expr>();
   if (isa<NullStmt>(Body))
     getCurCompoundScope().setHasEmptyLoopBodies();
 

clang/test/Sema/for.c

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 // Check C99 6.8.5p3
 void b1 (void) { for (void (*f) (void);;); }
 void b2 (void) { for (void f (void);;); }   // expected-error {{declaration of non-local variable}}
 void b3 (void) { for (static int f;;); }    // expected-error {{declaration of non-local variable}}
 void b4 (void) { for (typedef int f;;); }   // expected-error {{declaration of non-local variable}}
+void b5 (void) { for (struct { int i; } s;;); }
+void b6 (void) { for (enum { zero, ten = 10 } i;;); }