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

fix(python): fix comparison to True/False #91858

Closed
wants to merge 2 commits into from
Closed

Conversation

e-kwsm
Copy link
Contributor

@e-kwsm e-kwsm commented May 11, 2024

From PEP8 (https://peps.python.org/pep-0008/#programming-recommendations):

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

@e-kwsm e-kwsm requested a review from JDevlieghere as a code owner May 11, 2024 15:13
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category lldb clang:static analyzer openmp:libomp OpenMP host runtime openmp:libomptarget OpenMP offload runtime labels May 11, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 11, 2024

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-lldb

Author: Eisuke Kawashima (e-kwsm)

Changes

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

18 Files Affected:

  • (modified) clang/tools/scan-build/bin/set-xcode-analyzer (+1-1)
  • (modified) clang/utils/check_cfc/check_cfc.py (+2-2)
  • (modified) lldb/examples/python/crashlog.py (+1-1)
  • (modified) lldb/examples/python/disasm-stress-test.py (+2-2)
  • (modified) lldb/examples/summaries/cocoa/CFString.py (+3-3)
  • (modified) lldb/examples/summaries/pysummary.py (+1-1)
  • (modified) lldb/examples/synthetic/bitfield/example.py (+1-1)
  • (modified) lldb/packages/Python/lldbsuite/test/lldbtest.py (+1-1)
  • (modified) lldb/test/API/commands/command/script/welcome.py (+1-1)
  • (modified) lldb/test/API/commands/expression/call-throws/TestCallThatThrows.py (+3-3)
  • (modified) lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py (+1-1)
  • (modified) lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py (+1-1)
  • (modified) lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py (+1-1)
  • (modified) lldb/test/API/tools/lldb-server/TestLldbGdbServer.py (+1-1)
  • (modified) llvm/utils/indirect_calls.py (+1-1)
  • (modified) openmp/libompd/gdb-plugin/ompd/ompd.py (+1-1)
  • (modified) openmp/tools/archer/tests/lit.cfg (+1-1)
  • (modified) polly/lib/External/isl/imath/tests/gmp-compat-test/genpytest.py (+1-1)
diff --git a/clang/tools/scan-build/bin/set-xcode-analyzer b/clang/tools/scan-build/bin/set-xcode-analyzer
index f8c3f775ef7de..8d797bee4ae3b 100755
--- a/clang/tools/scan-build/bin/set-xcode-analyzer
+++ b/clang/tools/scan-build/bin/set-xcode-analyzer
@@ -107,7 +107,7 @@ def main():
     foundSpec = True
     ModifySpec(x, isBuiltinAnalyzer, path)
 
-  if foundSpec == False:
+  if foundSpec is False:
       print "(-) No compiler configuration file was found.  Xcode's analyzer has not been updated."
 
 if __name__ == '__main__':
diff --git a/clang/utils/check_cfc/check_cfc.py b/clang/utils/check_cfc/check_cfc.py
index 27d732d91030c..d4ddcb5bbb6a3 100755
--- a/clang/utils/check_cfc/check_cfc.py
+++ b/clang/utils/check_cfc/check_cfc.py
@@ -156,7 +156,7 @@ def get_output_file(args):
         elif arg.startswith("-o"):
             # Specified conjoined with -o
             return arg[2:]
-    assert grabnext == False
+    assert grabnext is False
 
     return None
 
@@ -182,7 +182,7 @@ def replace_output_file(args, new_name):
     if replaceidx is None:
         raise Exception
     replacement = new_name
-    if attached == True:
+    if attached is True:
         replacement = "-o" + new_name
     args[replaceidx] = replacement
     return args
diff --git a/lldb/examples/python/crashlog.py b/lldb/examples/python/crashlog.py
index 641b2e64d53b1..62bd73643a46e 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -166,7 +166,7 @@ def dump_symbolicated(self, crash_log, options):
             this_thread_crashed = self.app_specific_backtrace
             if not this_thread_crashed:
                 this_thread_crashed = self.did_crash()
-                if options.crashed_only and this_thread_crashed == False:
+                if options.crashed_only and this_thread_crashed is False:
                     return
 
             print("%s" % self)
diff --git a/lldb/examples/python/disasm-stress-test.py b/lldb/examples/python/disasm-stress-test.py
index 2d3314ee8e8ff..8487b24fdcba7 100755
--- a/lldb/examples/python/disasm-stress-test.py
+++ b/lldb/examples/python/disasm-stress-test.py
@@ -95,13 +95,13 @@ def GetLLDBFrameworkPath():
 
 debugger = lldb.SBDebugger.Create()
 
-if debugger.IsValid() == False:
+if debugger.IsValid() is False:
     print("Couldn't create an SBDebugger")
     sys.exit(-1)
 
 target = debugger.CreateTargetWithFileAndArch(None, arg_ns.arch)
 
-if target.IsValid() == False:
+if target.IsValid() is False:
     print("Couldn't create an SBTarget for architecture " + arg_ns.arch)
     sys.exit(-1)
 
diff --git a/lldb/examples/summaries/cocoa/CFString.py b/lldb/examples/summaries/cocoa/CFString.py
index 13c294ca34122..fdea2c48870cb 100644
--- a/lldb/examples/summaries/cocoa/CFString.py
+++ b/lldb/examples/summaries/cocoa/CFString.py
@@ -253,9 +253,9 @@ def get_child_at_index(self, index):
             elif (
                 self.inline
                 and self.explicit
-                and self.unicode == False
-                and self.special == False
-                and self.mutable == False
+                and self.unicode is False
+                and self.special is False
+                and self.mutable is False
             ):
                 return self.handle_inline_explicit()
             elif self.unicode:
diff --git a/lldb/examples/summaries/pysummary.py b/lldb/examples/summaries/pysummary.py
index e63a0bff56a13..666b9095f6b38 100644
--- a/lldb/examples/summaries/pysummary.py
+++ b/lldb/examples/summaries/pysummary.py
@@ -2,7 +2,7 @@
 
 
 def pyobj_summary(value, unused):
-    if value is None or value.IsValid() == False or value.GetValueAsUnsigned(0) == 0:
+    if value is None or value.IsValid() is False or value.GetValueAsUnsigned(0) == 0:
         return "<invalid>"
     refcnt = value.GetChildMemberWithName("ob_refcnt")
     expr = "(char*)PyString_AsString( (PyObject*)PyObject_Str( (PyObject*)0x%x) )" % (
diff --git a/lldb/examples/synthetic/bitfield/example.py b/lldb/examples/synthetic/bitfield/example.py
index 2f58123268aa1..0cdcdb1ba3c9d 100644
--- a/lldb/examples/synthetic/bitfield/example.py
+++ b/lldb/examples/synthetic/bitfield/example.py
@@ -51,7 +51,7 @@ def get_child_at_index(self, index):
             return None
         if index > self.num_children():
             return None
-        if self.valobj.IsValid() == False:
+        if self.valobj.IsValid() is False:
             return None
         if index == 0:
             return self.valobj.GetChildMemberWithName("value")
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 5fd686c143e9f..77d8dcd41f04e 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2446,7 +2446,7 @@ def found_str(matched):
                 log_lines.append(pattern_line)
 
                 # Convert to bool because match objects
-                # are True-ish but != True itself
+                # are True-ish but is not True itself
                 matched = bool(matched)
                 if matched != matching:
                     break
diff --git a/lldb/test/API/commands/command/script/welcome.py b/lldb/test/API/commands/command/script/welcome.py
index c1ae0063a52a9..12587114db2b2 100644
--- a/lldb/test/API/commands/command/script/welcome.py
+++ b/lldb/test/API/commands/command/script/welcome.py
@@ -45,7 +45,7 @@ def print_wait_impl(debugger, args, result, dict):
 def check_for_synchro(debugger, args, result, dict):
     if debugger.GetAsync():
         print("I am running async", file=result)
-    if debugger.GetAsync() == False:
+    if debugger.GetAsync() is False:
         print("I am running sync", file=result)
 
 
diff --git a/lldb/test/API/commands/expression/call-throws/TestCallThatThrows.py b/lldb/test/API/commands/expression/call-throws/TestCallThatThrows.py
index b8cc87c93ba61..9e2b0202ad2b5 100644
--- a/lldb/test/API/commands/expression/call-throws/TestCallThatThrows.py
+++ b/lldb/test/API/commands/expression/call-throws/TestCallThatThrows.py
@@ -61,7 +61,7 @@ def call_function(self):
 
         value = frame.EvaluateExpression("[my_class callMeIThrow]", options)
 
-        self.assertTrue(value.IsValid() and value.GetError().Success() == False)
+        self.assertTrue(value.IsValid() and value.GetError().Success() is False)
         self.check_after_call()
 
         # Now set the ObjC language breakpoint and make sure that doesn't
@@ -76,7 +76,7 @@ def call_function(self):
 
         value = frame.EvaluateExpression("[my_class callMeIThrow]", options)
 
-        self.assertTrue(value.IsValid() and value.GetError().Success() == False)
+        self.assertTrue(value.IsValid() and value.GetError().Success() is False)
         self.check_after_call()
 
         # Now turn off exception trapping, and call a function that catches the exceptions,
@@ -95,5 +95,5 @@ def call_function(self):
         options.SetUnwindOnError(False)
         value = frame.EvaluateExpression("[my_class callMeIThrow]", options)
 
-        self.assertTrue(value.IsValid() and value.GetError().Success() == False)
+        self.assertTrue(value.IsValid() and value.GetError().Success() is False)
         self.check_after_call()
diff --git a/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py b/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
index da9ce1b87d333..18e9077080a83 100644
--- a/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
+++ b/lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py
@@ -57,7 +57,7 @@ def disassemble_check_for_hi_and_foo(self, target, func, binaryname):
                 found_hi_string = True
             if "foo" in i.GetComment(target):
                 found_foo = True
-        if found_hi_string == False or found_foo == False:
+        if found_hi_string is False or found_foo is False:
             print(
                 'Did not find "HI" string or "foo" in disassembly symbolication in %s'
                 % binaryname
diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py b/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py
index a51b228e917cc..ce8742456f6f8 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py
@@ -61,7 +61,7 @@ def qXferRead(self, obj, annex, offset, length):
         wp_opts = lldb.SBWatchpointOptions()
         wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify)
         wp = target.WatchpointCreateByAddress(0x100, 8, wp_opts, err)
-        if self.TraceOn() and (err.Fail() or wp.IsValid == False):
+        if self.TraceOn() and (err.Fail() or wp.IsValid is False):
             strm = lldb.SBStream()
             err.GetDescription(strm)
             print("watchpoint failed: %s" % strm.GetData())
diff --git a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
index d770447f0771c..03649e0fbaf6e 100644
--- a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -39,7 +39,7 @@ def check_simulator_ostype(self, sdk, platform_name, arch=platform.machine()):
             for device in devices:
                 if "availability" in device and device["availability"] != "(available)":
                     continue
-                if "isAvailable" in device and device["isAvailable"] != True:
+                if "isAvailable" in device and device["isAvailable"] is not True:
                     continue
                 if deviceRuntime and runtime < deviceRuntime:
                     continue
diff --git a/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py b/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
index 32b36bc04c1a3..7275e55c58394 100644
--- a/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ b/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -953,7 +953,7 @@ def breakpoint_set_and_remove_work(self, want_hardware):
         z_packet_type = 0
 
         # If hardware breakpoint is requested set packet type to Z1
-        if want_hardware == True:
+        if want_hardware is True:
             z_packet_type = 1
 
         self.reset_test_sequence()
diff --git a/llvm/utils/indirect_calls.py b/llvm/utils/indirect_calls.py
index 2bdabc8c4d74f..2e2191ea58762 100755
--- a/llvm/utils/indirect_calls.py
+++ b/llvm/utils/indirect_calls.py
@@ -31,7 +31,7 @@ def look_for_indirect(file):
 
     function = ""
     for line in stdout.splitlines():
-        if line.startswith(" ") == False:
+        if line.startswith(" ") is False:
             function = line
         result = re.search("(call|jmp).*\*", line)
         if result != None:
diff --git a/openmp/libompd/gdb-plugin/ompd/ompd.py b/openmp/libompd/gdb-plugin/ompd/ompd.py
index a404e621e77bb..a0179a5d846dd 100644
--- a/openmp/libompd/gdb-plugin/ompd/ompd.py
+++ b/openmp/libompd/gdb-plugin/ompd/ompd.py
@@ -50,7 +50,7 @@ def invoke(self, arg, from_tty):
                     "No ompd_dll_locations symbol in execution, make sure to have an OMPD enabled OpenMP runtime"
                 )
 
-            while gdb.parse_and_eval("(char**)ompd_dll_locations") == False:
+            while gdb.parse_and_eval("(char**)ompd_dll_locations") is False:
                 gdb.execute("tbreak ompd_dll_locations_valid")
                 gdb.execute("continue")
 
diff --git a/openmp/tools/archer/tests/lit.cfg b/openmp/tools/archer/tests/lit.cfg
index 692cbfe97cf1e..0a4f848ef2869 100644
--- a/openmp/tools/archer/tests/lit.cfg
+++ b/openmp/tools/archer/tests/lit.cfg
@@ -83,7 +83,7 @@ if config.operating_system == 'Darwin':
 if 'Linux' in config.operating_system:
     config.available_features.add("linux")
 
-if config.has_tsan == True:
+if config.has_tsan is True:
     config.available_features.add("tsan")
 
 # to run with icc INTEL_LICENSE_FILE must be set
diff --git a/polly/lib/External/isl/imath/tests/gmp-compat-test/genpytest.py b/polly/lib/External/isl/imath/tests/gmp-compat-test/genpytest.py
index 1b5a38ce829b3..c331812309e24 100644
--- a/polly/lib/External/isl/imath/tests/gmp-compat-test/genpytest.py
+++ b/polly/lib/External/isl/imath/tests/gmp-compat-test/genpytest.py
@@ -54,7 +54,7 @@ def run_test(test, line, name, gmp_test_so, imath_test_so, *args):
   if childpid == 0:
     eq = test(line, name, gmp_test_so, imath_test_so, *args)
     if fork:
-      sys.exit(eq != True)
+      sys.exit(eq is not True)
     else:
       return eq
   else:

Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

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

LGTM

The PR summary is empty, but it looks like the commit message has an explanation for this change, so that's fine.

@DavidSpickett
Copy link
Collaborator

If this is split out from the other larger PR, should there be clang/ changes in here?

I've copied your commit message into the PR description, because with the way llvm is setup, we use the PR's description as the commit message for a squashed version of the changes.

(maybe Github uses the commit message if there's only one and the PR description is blank, but I wouldn't bet on it)

@JDevlieghere
Copy link
Member

If this is split out from the other larger PR, should there be clang/ changes in here?

+1, please unstage the clang and openmp changes.

Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

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

Hmm, actually, I'm not so sure about this change anymore. I went through PEP8 again and saw this:

Don’t compare boolean values to True or False using ==:
# Correct:
if greeting:
# Wrong:
if greeting == True:

Worse:
# Wrong:
if greeting is True:

This doesn't seem like the right change then, no?

@NagyDonat
Copy link
Contributor

NagyDonat commented May 15, 2024

The main problem with comparison to True/False is that it's completely redundant when the variable is guaranteed to be boolean. However, if a variable may contain either a boolean or something else, it's reasonable to compare it with True or False.

For the operator == there is another pitfall that according to Python True == 1 and False == 0 holds. (In ancient versions True and False were simply integer constants with these values; now bool is a subclass of int and preserves this old behavior.)

This implies that:

  • When x is guaranteed to be a boolean, it should be used as if x: or if not x: (instead of == or is checks).
  • Otherwise it may be reasonable to use the is operator: e.g. when x may be True, False or None, it is reasonable to check if x is False: or if x is None:.
  • Using == has no advantage over is and could cause very surprising bugs when the variable can hold either a boolean or a number, so I'd say that it should be avoided. (However I admit that when x is known to be either True, False or None, there's a high chance that I'd instinctively write if x == False:.)

@JDevlieghere
Copy link
Member

I don't think Alex is arguing in favor of keeping the old (wrong) behavior, but the first file looks like this:

foundSpec = False
if [...]
  foundSpec = True
[...]
if foundSpec is False:

It's pretty obvious this is a boolean and should use if not foundSpec.

from PEP8 (https://peps.python.org/pep-0008/#programming-recommendations):

> Comparisons to singletons like None should always be done with is or
> is not, never the equality operators.
@e-kwsm e-kwsm closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category lldb openmp:libomp OpenMP host runtime openmp:libomptarget OpenMP offload runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants