Skip to content

use bit shift to set bitflags #2 #8429

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

use bit shift to set bitflags #2 #8429

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions ext/standard/flock_compat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,18 @@
PHPAPI int php_flock(int fd, int operation);

#ifndef HAVE_FLOCK
# define LOCK_SH 1
# define LOCK_EX 2
# define LOCK_NB 4
# define LOCK_UN 8
# define LOCK_SH (1 << 0)
# define LOCK_EX (1 << 1)
# define LOCK_NB (1 << 2)
# define LOCK_UN (1 << 3)
PHPAPI int flock(int fd, int operation);
#endif

/* Userland LOCK_* constants */
#define PHP_LOCK_SH 1
#define PHP_LOCK_EX 2
#define PHP_LOCK_UN 3
#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))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

@iluuu1994 iluuu1994 Apr 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah would be nice to get LOCK_UN fixed, but in the meantime i still think this PR should be merged ^^

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@divinity76 divinity76 Apr 24, 2022 •
edited by Girgias
Loading

Choose a reason for hiding this comment

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

@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/

Copy link
Member

Choose a reason for hiding this comment

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

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.

#define PHP_LOCK_NB (1 << 2)

#ifdef PHP_WIN32
# ifdef EWOULDBLOCK
Expand Down