Zend: Refactor object property write API #11387
Draft
+406
−207
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3339dfe to
ffcaba5
Compare
iluuu1994
reviewed
Jun 8, 2023
| add_property_stringl(value, "orgtable", (field->org_table ? field->org_table : ""), field->org_table_length); | ||
| add_property_stringl(value, "def", (field->def ? field->def : ""), field->def_length); | ||
| add_property_stringl(value, "db", (field->db ? field->db : ""), field->db_length); | ||
| static void php_add_field_properties(zval *z_object, const MYSQL_FIELD *field) |
There was a problem hiding this comment.
This does not look like an improvement 🤷♂️ What's bad about the old API for these cases (except for the name)?
There was a problem hiding this comment.
The main issue is that the _str() variant takes ownership of the zend_string* value.
The other motivation is to be able to possibly use known zend_strings for the property name access.
There was a problem hiding this comment.
Transferring ownership can be useful to avoid unnecessary ADDREF/DELREF calls. It would be nice if function names indicated this, since it's not possible through the signature. Having variations for zend_string * names is useful. It being semantically different in terms of refcounting feels weird.
…as a Zend string Also fix type for bool version and add missing zend_string value for static properties.
As those functions effectively did the same thing just with a slightly different signature Except the add_property_str() version unintentionally took ownership of the zend_string and released it Maintain this awkward behaviour for the time being.
Array version usage: https://sourcegraph.com/search?q=context:global+lang:C+lang:C%2B%2B+add_property_array+-file:Zend/zend_API%5C.h&patternType=standard&sm=1&groupBy=repo Results are unrelated. Object version usage: https://sourcegraph.com/search?q=context:global+lang:C+lang:C%2B%2B+add_property_object+-file:Zend/zend_API%5C.h&patternType=standard&sm=1&groupBy=repo Results are unrelated. Reference version: https://sourcegraph.com/search?q=context:global+lang:C+lang:C%2B%2B+add_property_reference+-file:Zend/zend_API%5C.h&patternType=standard&sm=1&groupBy=repo No results.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Each commit is independent and should be reviewed in order.
The motivation for the first commit comes while I was working on #11381, as a lot of C string usage that would be prime candidates to be converted to known Zend strings were used when trying to update a property, which does not have an API to update a property name directly if one already has the name as a Zend string.
Commit number 2 comes from the realisation that we have a duplicated API which is mostly unused, so converted it to use the more standard
zend_update_property_TYPE()API.Commit number 3 removes some unused APIs which has been cross-checked using https://sourcegraph.com