Page MenuHomePhabricator

[clang][PR51931] Enable `-Wdeclaration-after-statement` for all C versions
ClosedPublic

Authored by zero9178 on Nov 30 2021, 3:12 AM.

Details

Summary

-Wdeclaration-after-statement currently only outputs an diagnostic if the user is compiling in C versions older than C99, even if the warning was explicitly requested by the user.
This patch makes the warning also available in later C versions. If the C version is C99 or later it is simply a normal warning that is disabled by default (as it is valid C99) and has to be enabled by users. In older versions it remains an extension warning, and therefore affected by -pedantic.

The above behaviour also matches GCCs behaviour.

Fixes https://bugs.llvm.org/show_bug.cgi?id=51931

Diff Detail

Event Timeline

zero9178 requested review of this revision.Nov 30 2021, 3:12 AM
zero9178 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 3:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

https://reviews.llvm.org/D115094 is a review for doing effectively the same fix. Can you coordinate with the other patch author to determine who will drive this fix?

https://reviews.llvm.org/D115094 is a review for doing effectively the same fix. Can you coordinate with the other patch author to determine who will drive this fix?

I sent an email to the author on December 13th but have yet to hear back from them. Should we move forward with this version of the patch then?

https://reviews.llvm.org/D115094 is a review for doing effectively the same fix. Can you coordinate with the other patch author to determine who will drive this fix?

I sent an email to the author on December 13th but have yet to hear back from them. Should we move forward with this version of the patch then?

I'd like to give a little bit more time (and another ping) only because of the recent holiday season, but I think we don't need to wait too much longer. I'm still digging out from under my pile of reviews, so I'd not be surprised if others are in a similar boat.

How about we proceed with this patch if there's no response by Mon Jan 10?

In the meantime, I have some minor feedback.

clang/test/Sema/warn-mixed-decls.c
2–5

I'd also like to see RUN lines for when we expect the diagnostic to not be enabled:

/* RUN: %clang_cc1 -fsyntax-only -verify=none -std=c99 %s */
/* RUN: %clang_cc1 -fsyntax-only -verify=none -x c++ %s */
/* RUN: %clang_cc1 -fsyntax-only -verify=none -x c++ -Wdeclaration-after-statement %s */

/* none-no-diagnostics */

I should note that the last RUN line will give different behavior between Clang and GCC: https://godbolt.org/z/o1PKo7dhM, but I think that's a more general issue that doesn't need to be addressed in this patch. (We don't have a way to flag a diagnostic as requiring a particular language mode.)

zero9178 updated this revision to Diff 397857.Thu, Jan 6, 5:15 AM

Addressed reviewer comments: Added tests for cases when the diagnostic should not be emitted.

zero9178 marked an inline comment as done.Thu, Jan 6, 5:17 AM
zero9178 added inline comments.
clang/test/Sema/warn-mixed-decls.c
2–5

The */ on the next line seems to be necessary so as lit seems to otherwise add */ to the command line of the RUN command. At least this is the case on my Windows machine.

aaron.ballman added inline comments.Thu, Jan 6, 5:42 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9863–9864

In the other review, I left a comment about the diagnostic text: https://reviews.llvm.org/D115094#3176985

Since we're cleaning this up, I think we should reword this diagnostic so it follows newer conventions. I think the extension diagnostic should read: mixing declarations and code is a C99 extension and the default ignore warning should read mixing declarations and code is incompatible with standards before C99. (This also helpfully removes the ISO C90 wording, which is confused about the name of the standard.)

Typically, we'd put the default ignore warning under a new CPre99Compat diagnostic group (spelled pre-c99-compat) as we do with other precompat diagnostics, but the goal here is to match GCC's behavior and so the existing warning group seems fine to me (I don't think we want warnings in multiple groups, but that's possibly an option if it matters in the future).

clang/test/Sema/warn-mixed-decls.c
2–5

Huh, today I learned. :-D Thanks for letting me know!

zero9178 updated this revision to Diff 397971.Thu, Jan 6, 1:01 PM
zero9178 marked an inline comment as done.

Improved error messages and adjusted tests accordingly

zero9178 added inline comments.Thu, Jan 6, 1:02 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9863–9864

A CPre99Compat diagnostic group sounds like a good idea if more such warnings would be requested by users.

aaron.ballman accepted this revision.Wed, Jan 12, 8:26 AM

LGTM! Thanks for the improvement!

This revision is now accepted and ready to land.Wed, Jan 12, 8:26 AM