This is an archive of the discontinued LLVM Phabricator instance.

Fix -Wdeclaration-after-statement doesn't work when used with -std=c99
Needs ReviewPublic

Authored by ram7bhupal on Dec 4 2021, 1:40 AM.

Details

Summary

-Wdeclaration-after-statement doesn't do anything if combined with -std=c99 or newer.

Take a look at the following program:

// prog.c
#include <stdio.h>

int main(void)
{

printf("hello world\n");
int i = 0;

return 0;

}

If I compile it with clang with the following command:

$ clang -std=c99 -Wdeclaration-after-statement prog.c

it produces no warnings.

If I compile the same code with gcc with the following command:

$ gcc -std=c99 -Wdeclaration-after-statement prog.c

it produces the following warning:

prog.c: In function ‘main’:
prog.c:6:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

6 |         int i = 0;
  |         ^~~

This is the behavior I would like to have with clang, but it only produces this warning if I use it with -std=c90 or -std=c89 or -ansi, like this:

$ clang -std=c90 -Wdeclaration-after-statement prog.c
prog.c:6:6: warning: ISO C90 forbids mixing declarations and code [-Wdeclaration-after-statement]

int i = 0;
    ^

1 warning generated.

Diff Detail

Event Timeline

ram7bhupal requested review of this revision.Dec 4 2021, 1:40 AM
ram7bhupal created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2021, 1:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This'll at least need a test case added - though the specifics of how the warning should work I'll leave up to @aaron.ballman - unless he wants some second opinions. (I don't immediately have a strong feeling either way regarding the current or proposed behavior)

I'm not opposed to the idea of issuing this diagnostic when it's explicitly enabled, but the changes aren't quite correct. ext_mixed_decls_code is defined as an Extension in DiagnosticSemaKinds.td, which means that these changes will cause us to issue a pedantic diagnostic in C99 and later mode now. We should not be issuing this as an extension warning in those modes.

The usual approach we have for this sort of thing is to issue two warnings:

  • "blah is a C99 extension" (issued as an extension warning when compiling in a mode earlier than C99)
  • "blah is incompatible with standards before C99" (issued when explicitly opting into compatibility warnings)

e.g., https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L575 and https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L572

Also, as @dblaikie points out, the changes are missing test coverage.

This'll at least need a test case added - though the specifics of how the warning should work I'll leave up to @aaron.ballman - unless he wants some second opinions. (I don't immediately have a strong feeling either way regarding the current or proposed behavior)

I'm not opposed to the idea of issuing this diagnostic when it's explicitly enabled, but the changes aren't quite correct. ext_mixed_decls_code is defined as an Extension in DiagnosticSemaKinds.td, which means that these changes will cause us to issue a pedantic diagnostic in C99 and later mode now. We should not be issuing this as an extension warning in those modes.

The usual approach we have for this sort of thing is to issue two warnings:

  • "blah is a C99 extension" (issued as an extension warning when compiling in a mode earlier than C99)
  • "blah is incompatible with standards before C99" (issued when explicitly opting into compatibility warnings)

e.g., https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L575 and https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticSemaKinds.td#L572

Also, as @dblaikie points out, the changes are missing test coverage.

There is already a revision uploaded for this: https://reviews.llvm.org/D114787 by @zero9178.

There is already a revision uploaded for this: https://reviews.llvm.org/D114787 by @zero9178.

Thanks for pointing this out! I thought this seemed familiar. :-D The patch author should coordinate to determine who drives one of these patches forward.