Convert IMAP resource to object #6418
Conversation
|
Maybe require users to call |
Girgias
force-pushed
the
imap-resource-to-object
branch
from
8fd905f to
8f66db9
Compare
I figured it out, was trying to dereference a NULL pointer or something alike. Should be working now. |
Girgias
force-pushed
the
imap-resource-to-object
branch
from
8f66db9 to
9dd9f90
Compare
Girgias
force-pushed
the
imap-resource-to-object
branch
from
9dd9f90 to
ea99c00
Compare
| { | ||
| pils *imap_le_struct = (pils *)rsrc->ptr; | ||
| static inline zend_object *imap_object_to_zend_object(php_imap_object *obj) { | ||
| return ((zend_object*)(obj + 1)) - 1; |
There was a problem hiding this comment.
These pointers are supposed to have the same size, so this could be simplified to return (zend_object*)obj. That looks wrong, though. Why not return obj->std?
There was a problem hiding this comment.
NGL, I have no idea, I just copy pasted this from Sara's PR converting (IIRC) FTP resources to object and it was doing that.
But if it can be simplified than I'll do that.
There was a problem hiding this comment.
For reference about the code I'm talking about: https://github.com/php/php-src/pull/5945/files#diff-526bea3bd44c00ccf1676ead9aeaac72a22e780107a86e0bebe3b81e5c67fb68R66-R73
There was a problem hiding this comment.
They're not the same size. sizeof(php_imap_object) is larger than sizeof(zend_object) by definition because the former contains an instance of the latter.
|-------------- php_imap_object ----------------|
|-- imap specific data --|-- zend_object data --|
obj+1 takes us from the left edge of the above diagram to the right edge, then the cast followed by the -1 brings us into the middle of it (and thus the start of the zend_object struct).
We could do return &(obj->std); but when reversing this there's no such convenient expression, so I like to use the math which is visibly reversible in both helpers.
There was a problem hiding this comment.
Ah, of course! Thanks for the explanation. :)
| if (!(imap_le_struct->flags & OP_PROTOTYPE)) { | ||
| mail_close_full(imap_le_struct->imap_stream, imap_le_struct->flags); | ||
| static inline php_imap_object *imap_object_from_zend_object(zend_object *zobj) { | ||
| return ((php_imap_object*)(zobj + 1)) - 1; |
There was a problem hiding this comment.
This has basically the same issue as above, but to convert from zend_object* to php_imap_object* one usually uses the XtOffsetOf macro.
There was a problem hiding this comment.
And again, because they're not the same size structs, the same answer applies.
XtOffsetOf() does the same math presented in my boilerplate helpers, but presents the (honestly kinda ugly) hack of intermediately casting to char*. c'mon, that's gross.
There was a problem hiding this comment.
But that's how it's documented. :)
There was a problem hiding this comment.
Eh. Lots of documentation is wrong about lots of things. ;)
| RETURN_FALSE; | ||
| } | ||
|
|
||
| if (imap_le_struct->imap_stream && imap_le_struct->imap_stream->mailbox) { | ||
| if (imap_conn_struct->imap_stream && imap_conn_struct->imap_stream->mailbox) { | ||
| rfc822_date(date); | ||
| object_init(return_value); |
There was a problem hiding this comment.
A subject for a future RFC and not something we should add here, but we should consider making this return value a formal type with declared/typed properties.
There was a problem hiding this comment.
Just to be sure, you mean adding an object e.g. IMAPMailboxProperties which would have the values as fixed properties instead of returning stdClass, correct?
There was a problem hiding this comment.
Yes. But again, something for a future project and shouldn't be mixed in with this one.
There was a problem hiding this comment.
Agreed, wouldn't want to take this longer to land.
Girgias
force-pushed
the
imap-resource-to-object
branch
from
ea99c00 to
fe46b60
Compare
Might be sub-optimal,
also I'm having an issue withimap_close()on how to propagate the EXPUNGE flag, so if anyone has an idea.