Partially refactor PgSql extension #6792
Conversation
Girgias
force-pushed
the
pgsql-refactor
branch
2 times, most recently
from
e9e36ad to
47e5687
Compare
ext/pgsql/pgsql.c
Outdated
| @@ -973,23 +972,22 @@ PHP_FUNCTION(pg_ping) | |||
| PHP_FUNCTION(pg_query) | |||
| { | |||
| zval *pgsql_link = NULL; | |||
| char *query; | |||
| zend_string *query; | |||
There was a problem hiding this comment.
Most of the zend_string changes don't make sense to me. Using zend_string makes sense if you actually use it as such, e.g. pass it around as zend_string, copy it, etc. Here you are just accepting the zend_string and then always using ZSTR_VAL, which imho is worse than the previous code.
There was a problem hiding this comment.
I was going on a top-down approach to start pass zend_strings to more internal functions, will continue to work on that and revisit the cases where that doesn't occur then.
There was a problem hiding this comment.
Right. It's mostly about cases like this, where a string is only used to talk to some 3rd-party C API (libpq), which is never going to accept zend_strings, so it just adds extra friction.
Girgias
force-pushed
the
pgsql-refactor
branch
2 times, most recently
from
7a588ee to
7a3f971
Compare
|
Looks okay, though I still think some of the zend_string changes aren't really improvements... |
| if (strchr(mode_string, '+') == mode_string+1) { | ||
| pgsql_mode |= INV_READ; | ||
| } | ||
| create = true; |
There was a problem hiding this comment.
Aren't you still missing create=true for the w+ case?
There was a problem hiding this comment.
Indeed, this does make this whole block still as messy as before but probably still clearer
Also use de_DE locale instead
New name is _php_pgsql_identifier_is_escaped()
|
I already was in the middle of a rebase trying to squash and reorganize the commits, so apologies for that. |
Mostly to use zend_string* more often and a couple of performance improvements by using interned strings.