Skip to content

[Clang] [Sema] Allow non-local/non-variable declarations in for loop #129737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ Bug Fixes in This Version
(#GH125500).
- Fixed clang crash when #embed data does not fit into an array
(#GH128987).
- Non-local variable and non-variable declarations in the first clause of a ``for`` loop in C are no longer incorrectly
considered an error in C23 mode and are allowed as an extension in earlier language modes.

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
17 changes: 17 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -10797,6 +10797,23 @@ def err_non_local_variable_decl_in_for : Error<
"declaration of non-local variable in 'for' loop">;
def err_non_variable_decl_in_for : Error<
"non-variable declaration in 'for' loop">;

def ext_c23_non_local_variable_decl_in_for : Extension<
"declaration of non-local variable in 'for' loop is a C23 extension">,
InGroup<C23>;

def warn_c17_non_local_variable_decl_in_for : Warning<
"declaration of non-local variable in 'for' loop is incompatible with C standards before C23">,
DefaultIgnore, InGroup<CPre23Compat>;

def ext_c23_non_variable_decl_in_for : Extension<
"non-variable declaration in 'for' loop is a C23 extension">,
InGroup<C23>;

def warn_c17_non_variable_decl_in_for : Warning<
"non-variable declaration in 'for' loop is incompatible with C standards before C23">,
DefaultIgnore, InGroup<CPre23Compat>;

def err_toomany_element_decls : Error<
"only one element declaration is allowed">;
def err_selector_element_not_lvalue : Error<
Expand Down
13 changes: 8 additions & 5 deletions clang/lib/Sema/SemaStmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2269,10 +2269,11 @@ StmtResult Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
for (auto *DI : DS->decls()) {
if (VarDecl *VD = dyn_cast<VarDecl>(DI)) {
VarDeclSeen = true;
if (VD->isLocalVarDecl() && !VD->hasLocalStorage()) {
Diag(DI->getLocation(), diag::err_non_local_variable_decl_in_for);
DI->setInvalidDecl();
}
if (VD->isLocalVarDecl() && !VD->hasLocalStorage())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a big fan of quoting the section of the standard that allows various features. This allows folks in the future to know where to look to better understand the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a C99 quote further up above on line 2264; we could add a comment that mentions that this changed at some point though. Something like ‘C23 has no such restriction’ maybe?

Diag(DI->getLocation(),
getLangOpts().C23
? diag::warn_c17_non_local_variable_decl_in_for
: diag::ext_c23_non_local_variable_decl_in_for);
} else if (!NonVarSeen) {
// Keep track of the first non-variable declaration we saw so that
// we can diagnose if we don't see any variable declarations. This
Expand All @@ -2284,7 +2285,9 @@ StmtResult Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
// Diagnose if we saw a non-variable declaration but no variable
// declarations.
if (NonVarSeen && !VarDeclSeen)
Diag(NonVarSeen->getLocation(), diag::err_non_variable_decl_in_for);
Diag(NonVarSeen->getLocation(),
getLangOpts().C23 ? diag::warn_c17_non_variable_decl_in_for
: diag::ext_c23_non_variable_decl_in_for);
}
}

Expand Down
22 changes: 15 additions & 7 deletions clang/test/Sema/for.c
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// RUN: %clang_cc1 -fsyntax-only -verify=c11 -std=c11 -pedantic %s
// RUN: %clang_cc1 -fsyntax-only -verify=c23 -std=c23 -Wpre-c23-compat %s

// Check C99 6.8.5p3
void b1 (void) { for (void (*f) (void);;); }
void b2 (void) { for (void f (void);;); } // expected-error {{non-variable declaration in 'for' loop}}
void b3 (void) { for (static int f;;); } // expected-error {{declaration of non-local variable}}
void b4 (void) { for (typedef int f;;); } // expected-error {{non-variable declaration in 'for' loop}}
void b2 (void) { for (void f (void);;); } /* c11-warning {{non-variable declaration in 'for' loop is a C23 extension}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these test should have show the non-local variables being used within the loops as well. Otherwise the tests are not fully testing the functionality and are not fully protecting against regressions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I can reference them in the loop condition I suppose.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the drive by after commit review but I really would like to see that. Having spent some time digging into regression the past few months a main pattern is lack of testing and so I am trying to push asking for more in the testing arena.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense; I’m a bit busy at the moment but I should be able to do that next week considering that it shouldn’t take too long.

c23-warning {{non-variable declaration in 'for' loop is incompatible with C standards before C23}} */
void b3 (void) { for (static int f;;); } /* c11-warning {{declaration of non-local variable in 'for' loop is a C23 extension}}
c23-warning {{declaration of non-local variable in 'for' loop is incompatible with C standards before C23}} */

void b4 (void) { for (typedef int f;;); } /* c11-warning {{non-variable declaration in 'for' loop is a C23 extension}}
c23-warning {{non-variable declaration in 'for' loop is incompatible with C standards before C23}} */
void b5 (void) { for (struct { int i; } s;;); }
void b6 (void) { for (enum { zero, ten = 10 } i;;); }
void b7 (void) { for (struct s { int i; };;); } // expected-error {{non-variable declaration in 'for' loop}}
void b8 (void) { for (static struct { int i; } s;;); } // expected-error {{declaration of non-local variable}}
void b7 (void) { for (struct s { int i; };;); } /* c11-warning {{non-variable declaration in 'for' loop is a C23 extension}}
c23-warning {{non-variable declaration in 'for' loop is incompatible with C standards before C23}} */
void b8 (void) { for (static struct { int i; } s;;); } /* c11-warning {{declaration of non-local variable in 'for' loop is a C23 extension}}
c23-warning {{declaration of non-local variable in 'for' loop is incompatible with C standards before C23}} */
void b9 (void) { for (struct { int i; } (*s)(struct { int j; } o) = 0;;); }
void b10(void) { for (typedef struct { int i; } (*s)(struct { int j; });;); } // expected-error {{non-variable declaration in 'for' loop}}
void b10(void) { for (typedef struct { int i; } (*s)(struct { int j; });;); } /* c11-warning {{non-variable declaration in 'for' loop is a C23 extension}}
c23-warning {{non-variable declaration in 'for' loop is incompatible with C standards before C23}} */