use bit shift to set bitflags #2 #8429
Conversation
nitpicking, this makes it practically impossible to accidentally use a number with 2 bits set in the future, it's also my personal preference, its trivial to see "LOCK_NB use bit 2" and that "the next unused bit is bit 4" here it also highlights that there is something strange about the userland LOCK_UN constant which has been (or is being?) discussed here php#8428
divinity76
changed the title
| #define PHP_LOCK_NB 4 | ||
| #define PHP_LOCK_SH (1 << 0) | ||
| #define PHP_LOCK_EX (1 << 1) | ||
| #define PHP_LOCK_UN ((1 << 0) | (1 << 1)) |
There was a problem hiding this comment.
Thank you! It's a bit unfortunate that PHP_LOCK_UN looks like an option combining PHP_LOCK_SH and PHP_LOCK_EX when it's really just a different option. Maybe this could be made more obvious by keeping these options continuous integers.
#define PHP_LOCK_SH 1
#define PHP_LOCK_EX 2
#define PHP_LOCK_UN 3
/* Can be combined with all of the above */
#define PHP_LOCK_NB (1 << 2)It's unfortunate that we have a different binary representation here.
There was a problem hiding this comment.
unfortunate indeed, as it breaks code like https://3v4l.org/0XT2P
(which is perfectly valid code in most other languages, like C)
Maybe this could be made more obvious by keeping these options continuous integers
they shouldn't continuous integers, they should be continuous bits, because they are in fact bitflag options, as they're supposed to be combined with LOCK_SH | LOCK_NB and LOCK_EX | LOCK_NB
(but afaik LOCK_UN is not supposed to be combined with LOCK_NB)
There was a problem hiding this comment.
they shouldn't continuous integers, they should be continuous bits, because they are in fact bitflag options, as they're supposed to be combined with LOCK_SH | LOCK_NB and LOCK_EX | LOCK_NB
Yes, I agree, they should be. Unfortunately in PHP they are not.
unfortunate indeed, as it breaks code like https://3v4l.org/0XT2P (which is perfectly valid code in most other languages, like C)
Yeah this is not great. The check ($flags & ~LOCK_NB) === LOCK_SH would be correct but obviously suboptimal. It might be worth addressing this in internals and potentially fix it in PHP 8.2.
There was a problem hiding this comment.
yeah would be nice to get LOCK_UN fixed, but in the meantime i still think this PR should be merged ^^
There was a problem hiding this comment.
No? This is clearly not a bitmask, so you shouldn't try to make it look like one. This is a two bit lock type and an NB flag -- writing the two-bit type as if it were a combination of independent bits doesn't make any sense.
There was a problem hiding this comment.
@nikic btw do you have an opinion on this? https://www.reddit.com/r/programminghorror/comments/uapuo7/php_invented_their_own_nonstandard_value_for_lock/
There was a problem hiding this comment.
That programmer should have better read the fine manual:
operation is one of the following:
LOCK_SH to acquire a shared lock (reader).
LOCK_EX to acquire an exclusive lock (writer).
LOCK_UN to release a lock (shared or exclusive).It is also possible to add LOCK_NB as a bitmask to one of the above operations, if flock() should not block during the locking attempt.
|
According to the discussion, it should be clear that this is not a bitmask, so we shouldn't make it look like one. Thanks for the PR nonetheless! |
nitpicking, this makes it practically impossible to accidentally use a number with 2 bits set in the future,
it's also my personal preference, its trivial to see "LOCK_NB use bit 2" and that "the next unused bit is bit 4"
here it also highlights that there is something strange about the userland LOCK_UN constant
which has been (or is being?) discussed here #8428