-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fixed bug #52015: Allow including end date in DatePeriod iterations #8550
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
Conversation
Can
|
They can be used together, so I will add a test case for that too. |
ext/date/php_date.h
Outdated
@@ -93,6 +93,7 @@ struct _php_period_obj { | |||
int recurrences; | |||
bool initialized; | |||
bool include_start_date; | |||
int include_end_date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should't this be a bool
? A test for recurrences
is missing, I think this would result in one too many iterations. Edit Oh actually that probably doesn't make sense since these two don't go together. Maybe we should inhibit them being used together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @derickr, both can be used together.
Anyhow, consider to use a bitfield here to safe some memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmb69 I was referring to using recurrences
and include_end_date
together which I don't think makes sense as recurrences determine the end date. Unless I'm missing something, I'm not familiar with DatePeriod
at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! And yes, these are mutually exclusive. Still, I think the three bool
s should be changed to bitfield.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the patch was originally made when the bool include_start_date
was still an int
.
recurrences
and include_end_date
together make no sense, but recurrences
and include_start_date
could perhaps. I'll have a look at seeing if anything needs addressing/excluding.
initialised
should not be part of the bit mask though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I have looked this, I am reluctant changing this to a bit mask, due to the negative of "exclude start date". It makes the rest of the code harder to read and understand, so I won't be changing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't referring to a bitmask, but rather a bitfield, i.e.
int initialized:1;
int include_start_date:1;
int include_end_date:1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh :-) — I would have hoped the compiler would do with a bool type. But it doesn't. Changing them to bool ...:1
does work, and is IMO prefered over int ...:1
:
struct _php_period_obj {
timelib_time * start; /* 0 8 */
zend_class_entry * start_ce; /* 8 8 */
timelib_time * current; /* 16 8 */
timelib_time * end; /* 24 8 */
timelib_rel_time * interval; /* 32 8 */
int recurrences; /* 40 4 */
_Bool initialized:1; /* 44: 0 1 */
_Bool include_start_date:1; /* 44: 1 1 */
_Bool include_end_date:1; /* 44: 2 1 */
/* XXX 5 bits hole, try to pack */
/* XXX 3 bytes hole, try to pack */
zend_object std; /* 48 56 */
/* size: 104, cachelines: 2, members: 10 */
/* sum members: 100, holes: 1, sum holes: 3 */
/* sum bitfield members: 3 bits, bit holes: 1, sum bit holes: 5 bits */
/* last cacheline: 40 bytes */
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it doesn't actually end up saving memory as the zend_object needs to be aligned to an 8 byte boundary ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right! (Integrating recurrences
in the bitfield would be an option, but doesn't look that it's worth it.)
…_date on PHP 8.2 (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [VarDumper][VarExporter] Deal with DatePeriod->include_end_date on PHP 8.2 | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Follows php/php-src#8550 Commits ------- 9319a7a [VarDumper][VarExporter] Deal with DatePeriod->include_end_date on PHP 8.2
Based on a patch by @degeberg (Daniel Egeberg) in https://bugs.php.net/bug.php?id=52015