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

Add new flag -Wpointer-integer-ordered-compare #88489

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

11happy
Copy link
Contributor

@11happy 11happy commented Apr 12, 2024

Overview:
This pull request fixes #88090 where a false negative issue related to -Wpointer-integer-compare failing on comparison between a pointer and zero.

Testing:

  • Tested the updated code.
  • Verified that other functionalities remain unaffected.

Dependencies:

  • No dependencies on other pull requests.

CC:

Signed-off-by: 11happy <soni5happy@gmail.com>
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 12, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 12, 2024

@llvm/pr-subscribers-clang

Author: Bhuminjay Soni (11happy)

Changes

Overview:
This pull request fixes #88090 where a false negative issue related to -Wpointer-integer-compare failing on comparison between a pointer and zero.

Testing:

  • Tested the updated code.
  • Verified that other functionalities remain unaffected.

Dependencies:

  • No dependencies on other pull requests.

CC:

  • @AaronBallman , @shafik

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

5 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+6)
  • (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2-1)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5-3)
  • (modified) clang/test/Misc/warning-flags.c (+2-3)
  • (added) clang/test/Sema/compare_pointers.c (+30)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 93318871fa9f6e..e205088c5cd440 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -245,6 +245,12 @@ Modified Compiler Flags
        f3 *c = (f3 *)x;
      }
 
+- Added a new diagnostic flag ``-Wpointer-integer-ordered-compare`` which is
+  grouped under ``-Wpointer-integer-compare`` and moved previously
+  ungrouped diagnostics ``ext_typecheck_ordered_comparison_of_pointer_integer``,
+  ``ext_typecheck_ordered_comparison_of_pointer_and_zero`` under
+  ``-Wpointer-integer-ordered-compare``. Fixes #GH88090
+
 
 Removed Compiler Flags
 -------------------------
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 47747d8704b6c8..2cf56574650839 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -714,10 +714,11 @@ def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
+def PointerIntegerOrderedCompare : DiagGroup<"pointer-integer-ordered-compare">;
+def PointerIntegerCompare : DiagGroup<"pointer-integer-compare",[PointerIntegerOrderedCompare]>;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;
 def XorUsedAsPow : DiagGroup<"xor-used-as-pow">;
-
 def Unsequenced : DiagGroup<"unsequenced">;
 // GCC name for -Wunsequenced
 def : DiagGroup<"sequence-point", [Unsequenced]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 180e913155d67c..a5632a3f6f61da 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7272,9 +7272,11 @@ def err_typecheck_sub_ptr_compatible : Error<
   "%diff{$ and $ are not pointers to compatible types|"
   "pointers to incompatible types}0,1">;
 def ext_typecheck_ordered_comparison_of_pointer_integer : ExtWarn<
-  "ordered comparison between pointer and integer (%0 and %1)">;
+  "ordered comparison between pointer and integer (%0 and %1)">,
+  InGroup<PointerIntegerOrderedCompare>;
 def ext_typecheck_ordered_comparison_of_pointer_and_zero : Extension<
-  "ordered comparison between pointer and zero (%0 and %1) is an extension">;
+  "ordered comparison between pointer and zero (%0 and %1) is an extension">,
+  InGroup<PointerIntegerOrderedCompare>;
 def err_typecheck_ordered_comparison_of_pointer_and_zero : Error<
   "ordered comparison between pointer and zero (%0 and %1)">;
 def err_typecheck_three_way_comparison_of_pointer_and_zero : Error<
@@ -7299,7 +7301,7 @@ def err_typecheck_comparison_of_fptr_to_void : Error<
   "equality comparison between function pointer and void pointer (%0 and %1)">;
 def ext_typecheck_comparison_of_pointer_integer : ExtWarn<
   "comparison between pointer and integer (%0 and %1)">,
-  InGroup<DiagGroup<"pointer-integer-compare">>;
+  InGroup<PointerIntegerCompare>;
 def err_typecheck_comparison_of_pointer_integer : Error<
   "comparison between pointer and integer (%0 and %1)">;
 def ext_typecheck_comparison_of_distinct_pointers : ExtWarn<
diff --git a/clang/test/Misc/warning-flags.c b/clang/test/Misc/warning-flags.c
index dd73331913c6f6..8c0e151613bd82 100644
--- a/clang/test/Misc/warning-flags.c
+++ b/clang/test/Misc/warning-flags.c
@@ -18,7 +18,7 @@ This test serves two purposes:
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (66):
+CHECK: Warnings without flags (65):
 
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
@@ -28,7 +28,6 @@ CHECK-NEXT:   ext_plain_complex
 CHECK-NEXT:   ext_template_arg_extra_parens
 CHECK-NEXT:   ext_template_spec_extra_headers
 CHECK-NEXT:   ext_typecheck_cond_incompatible_operands
-CHECK-NEXT:   ext_typecheck_ordered_comparison_of_pointer_integer
 CHECK-NEXT:   ext_using_undefined_std
 CHECK-NEXT:   pp_invalid_string_literal
 CHECK-NEXT:   pp_out_of_date_dependency
@@ -89,4 +88,4 @@ CHECK-NEXT:   warn_weak_import
 
 The list of warnings in -Wpedantic should NEVER grow.
 
-CHECK: Number in -Wpedantic (not covered by other -W flags): 26
+CHECK: Number in -Wpedantic (not covered by other -W flags): 25
diff --git a/clang/test/Sema/compare_pointers.c b/clang/test/Sema/compare_pointers.c
new file mode 100644
index 00000000000000..07592ea17a3193
--- /dev/null
+++ b/clang/test/Sema/compare_pointers.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -Wno-pointer-integer-compare -Wpointer-integer-ordered-compare -fsyntax-only -verify=pointer-integer-ordered %s
+// RUN: %clang_cc1 -Wpointer-integer-compare -Wno-pointer-integer-ordered-compare -fsyntax-only -verify=pointer-integer %s
+
+void test1(int *a){
+    int b = 1;
+    short c = 1;
+    if(c<a) {}; // pointer-integer-ordered-warning{{ordered comparison between pointer and integer ('short' and 'int *')}}
+    if(a!=b) {}; // pointer-integer-warning{{comparison between pointer and integer ('int *' and 'int')}}
+    if(a == b) {}; // pointer-integer-warning{{comparison between pointer and integer ('int *' and 'int')}}
+}
+
+int test2(int *a){
+    return a>=0; // pointer-integer-ordered-warning{{ordered comparison between pointer and zero ('int *' and 'int') is an extension}}
+}
+
+int test3(int *a){
+    return a>=1; // pointer-integer-ordered-warning{{ordered comparison between pointer and integer ('int *' and 'int')}}
+}
+
+int test4(int *a){
+    return a>1; // pointer-integer-ordered-warning{{ordered comparison between pointer and integer ('int *' and 'int')}}
+}
+int test5(int *a){
+    int zero = 0;
+    return a>=zero; // pointer-integer-ordered-warning{{ordered comparison between pointer and integer ('int *' and 'int')}}
+}
+
+
+
+

Signed-off-by: 11happy <soni5happy@gmail.com>
Copy link

@vincent-mailhol vincent-mailhol left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of the issue I created.

For what it is worth, here is my review. I am not a contributor of the llvm project, so I only left superficial nitpicks.

def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
def Varargs : DiagGroup<"varargs">;
def XorUsedAsPow : DiagGroup<"xor-used-as-pow">;

Choose a reason for hiding this comment

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

Nitpick: this line removal is not related to the commit.

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 248 to 252
- Added a new diagnostic flag ``-Wpointer-integer-ordered-compare`` which is
grouped under ``-Wpointer-integer-compare`` and moved previously
ungrouped diagnostics ``ext_typecheck_ordered_comparison_of_pointer_integer``,
``ext_typecheck_ordered_comparison_of_pointer_and_zero`` under
``-Wpointer-integer-ordered-compare``. Fixes #GH88090

Choose a reason for hiding this comment

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

Maybe add a sentence to say that this also resolves a false negative for comparison between pointers and literal zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 22 to 23
}
int test5(int *a){

Choose a reason for hiding this comment

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

Nitpick: add newline between functions

Suggested change
}
int test5(int *a){
}
int test5(int *a){

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: 11happy <soni5happy@gmail.com>
@11happy
Copy link
Contributor Author

11happy commented Apr 18, 2024

Hello @vincent-mailhol thank you for the review I have made the suggested changed.

@11happy
Copy link
Contributor Author

11happy commented Apr 18, 2024

humble reminder @AaronBallman , @shafik

@11happy
Copy link
Contributor Author

11happy commented Apr 26, 2024

Humble ping!

Copy link

@vincent-mailhol vincent-mailhol left a comment

Choose a reason for hiding this comment

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

As far as my understanding of the LLVM project goes, this change looks good to me. I am not an expert of this project, but regardless and for what it is worth, here is my approval (with one nitpick).

grouped under ``-Wpointer-integer-compare`` and moved previously
ungrouped diagnostics ``ext_typecheck_ordered_comparison_of_pointer_integer``,
``ext_typecheck_ordered_comparison_of_pointer_and_zero`` under
``-Wpointer-integer-ordered-compare``.This also resolves false negative

Choose a reason for hiding this comment

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

Nitpick: put a space after the period.

Suggested change
``-Wpointer-integer-ordered-compare``.This also resolves false negative
``-Wpointer-integer-ordered-compare``. This also resolves false negative

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.

[False negative] -Wpointer-integer-compare fails on comparison between a pointer and zero
3 participants