Skip to content
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

Carving out -Wformat warning about scoped enums into a subwarning #88595

Merged
merged 12 commits into from
Apr 22, 2024

Conversation

ZijunZhaoCCK
Copy link
Contributor

@ZijunZhaoCCK ZijunZhaoCCK commented Apr 13, 2024

Make it part of -Wformat-pedantic.

Fixes #81647

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 13, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 13, 2024

@llvm/pr-subscribers-clang

Author: None (ZijunZhaoCCK)

Changes

Make it part of -Wformat-pedantic.


Full diff: https://github.com/llvm/llvm-project/pull/88595.diff

3 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2-2)
  • (modified) clang/test/Sema/format-strings-scanf.c (+1-1)
  • (modified) clang/test/Sema/format-strings-signedness.c (+3-3)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 774d2b53a38252..4ba27d62208da4 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9827,7 +9827,7 @@ def warn_scanf_nonzero_width : Warning<
 def warn_format_conversion_argument_type_mismatch : Warning<
   "format specifies type %0 but the argument has "
   "%select{type|underlying type}2 %1">,
-  InGroup<Format>;
+  InGroup<FormatPedantic>;
 def warn_format_conversion_argument_type_mismatch_pedantic : Extension<
   warn_format_conversion_argument_type_mismatch.Summary>,
   InGroup<FormatPedantic>;
@@ -9840,7 +9840,7 @@ def warn_format_conversion_argument_type_mismatch_confusion : Warning<
 def warn_format_argument_needs_cast : Warning<
   "%select{values of type|enum values with underlying type}2 '%0' should not "
   "be used as format arguments; add an explicit cast to %1 instead">,
-  InGroup<Format>;
+  InGroup<FormatPedantic>;
 def warn_format_argument_needs_cast_pedantic : Warning<
   warn_format_argument_needs_cast.Summary>,
   InGroup<FormatPedantic>, DefaultIgnore;
diff --git a/clang/test/Sema/format-strings-scanf.c b/clang/test/Sema/format-strings-scanf.c
index eb5b8ec36bf7a4..9bdc46bfeebc3b 100644
--- a/clang/test/Sema/format-strings-scanf.c
+++ b/clang/test/Sema/format-strings-scanf.c
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify -Wformat-nonliteral %s
 
 // Test that -Wformat=0 works:
-// RUN: %clang_cc1 -std=c11 -fsyntax-only -Werror -Wformat=0 %s
+// RUN: %clang_cc1 -std=c11 -fsyntax-only -Werror -Wformat=0 -Wno-format-pedantic %s
 
 #include <stdarg.h>
 typedef __SIZE_TYPE__ size_t;
diff --git a/clang/test/Sema/format-strings-signedness.c b/clang/test/Sema/format-strings-signedness.c
index d5a8140d9ef8a0..ccd4d72c2d2635 100644
--- a/clang/test/Sema/format-strings-signedness.c
+++ b/clang/test/Sema/format-strings-signedness.c
@@ -12,8 +12,8 @@
 // RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -std=c11 -fsyntax-only -Wformat -verify=okay %s
 
 // Verify that -Wformat-signedness with -Wno-format are not reported (gcc compat).
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -std=c11 -fsyntax-only -Wformat-signedness -Wno-format -verify=okay %s
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -std=c11 -fsyntax-only -Wno-format -Wformat-signedness -verify=okay %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -std=c11 -fsyntax-only -Wformat-signedness -Wno-format -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -std=c11 -fsyntax-only -Wno-format -Wformat-signedness -verify %s
 // okay-no-diagnostics
 
 int printf(const char *restrict format, ...);
@@ -218,5 +218,5 @@ void test_printf_unsigned_priX16(uint16_t x) {
 void test_suppress(int x)
 {
 #pragma GCC diagnostic ignored "-Wformat"
-    printf("%u", x);
+    printf("%u", x); // expected-warning{{format specifies type 'unsigned int' but the argument has type 'int'}}
 }

@@ -12,8 +12,8 @@
// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -std=c11 -fsyntax-only -Wformat -verify=okay %s

// Verify that -Wformat-signedness with -Wno-format are not reported (gcc compat).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The -Wformat-signedness warning was implemented to work the same way as the same warning do in gcc. This patch seems to remove that gcc compatibility. With this patch -Wformat-signedness is now controlled by -Wformat-pedantic instead of -Wformat (as in gcc). I don't know how important it is for clang to be gcc compatible in this way (@AaronBallman what do you think?). At least this comment need to be updated as it no longer match the test below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this is intentional, see the discussion at: #81647 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That discussion is only about -Wformat scoped enums warnings, but this patch currently move all the -Wformat warnings regarding type mismatch into a subwarning, that a lot more warnings than only scoped enums. Is that intended?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, that seems like an outstanding issue with the PR and should be addressed. Only the scoped enumeration behavior should change.

clang/test/Sema/format-strings-signedness.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Please mention that this fixes #81647 in the patch summary so that the issue is automatically closed.

You should also add a release note so that users know about the change (and link to the issue being closed).

Copy link

github-actions bot commented Apr 16, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@pirama-arumuga-nainar
Copy link
Collaborator

Please mention that this fixes #81647 in the patch summary so that the issue is automatically closed.

You should also add a release note so that users know about the change (and link to the issue being closed).

This is also pending. The rest of the changes LGTM.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Please be sure to add a release note in clang/docs/ReleaseNotes.rst. The code changes themselves LGTM, but there are some issues with the tests.

clang/test/FixIt/format.cpp Show resolved Hide resolved
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM!

@ZijunZhaoCCK ZijunZhaoCCK merged commit 73ed215 into llvm:main Apr 22, 2024
5 checks passed
krispymb pushed a commit to apple/llvm-project that referenced this pull request Apr 24, 2024
…vm#88595)

Make it part of -Wformat-pedantic.

Fixes llvm#81647

(cherry picked from commit 73ed215)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carving out -Wformat warning about scoped enums into a subwarning
5 participants