This is an archive of the discontinued Mercurial Phabricator instance.

debugcommands: prevent using `is False`
ClosedPublic

Authored by pulkit on Feb 17 2021, 2:43 PM.

Details

Summary

I was touching this code in a future patch and marmoute warned about usage of
is False here.

Quoting marmoute:

"is False" is going to check if the object you have the very same object in
memory than the one Python allocated for False (in practice 0)
This will "mostly work" on cpython because of implementation details, but
is semantically wrong and can start breaking unexpectedly

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Feb 17 2021, 2:43 PM

I was touching this code in a future patch and marmoute warned about usage of is False here.
Quoting marmoute:

"is False" is going to check if the object you have the very same object in
memory than the one Python allocated for False (in practice 0)
This will "mostly work" on cpython because of implementation details, but
is semantically wrong and can start breaking unexpectedly

Is if foo == False valid in this case? I probably got my wires crossed with is None. (I'm fine with the code change here, just wondering for future reference.)

Alphare accepted this revision.Feb 23 2021, 4:20 AM
Alphare added a subscriber: Alphare.

Is if foo == False valid in this case? I probably got my wires crossed with is None. (I'm fine with the code change here, just wondering for future reference.)

You will probably always want to use if not foo instead. if is None checks against the None singleton, so it's fine, but there is no singleton (aside from implementation details as marmoute said) for booleans in Python.

marmoute accepted this revision.Mar 1 2021, 3:51 AM
marmoute added a subscriber: marmoute.

baymax updated this revision to Diff 26109.Mar 5 2021, 1:17 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.