Re: Improved zend_string API

> On Jun 29, 2015, at 7:37 PM, Bob Weinand <bobwei9@hotmail.com> wrote: > > Hey Anatol, > >> Am 29.06.2015 um 22:41 schrieb Anatol Belski <anatol.php@belski.net>: >> >> Hi Bob, >> >>> -----Original Message----- >>> From: Bob Weinand [mailto:bobwei9@hotmail.com] >>> Sent: Monday, June 29, 2015 6:44 PM >>> To: Dmitry Stogov >>> Cc: francois; Anatol Belski; Anatol Belski; PHP Internals >>> Subject: Re: [PHP-DEV] Improved zend_string API >>> >>> Hey, looks like I'm a bit late to the party, but I'd like to object against >>> the >>> accessor macros on zend_string. >>> >>> What does it help to replace very nice ->val, ->len etc. with macros? >>> >>> ZSTR_VAL(str)[ZSTR_LEN(str)] = 0; or str->val[str->len] = 0; >>> zend_string_set_len(str, ZSTR_LEN(str) - 2); or str->len -= 2; Also, that particular >>> function suggests to me that the string is expanded if lengths don't match. >>> >>> I much prefer direct accessors which indicate a) no side-effects happening here, >>> b) less functions to remember (you just look at the struct and know everything >>> about it) and c) end up being more readable. >>> >>> Yes, I'm totally aware that when we ever want to change the API (I don't see >>> coming it in near future anyway), it could help having wrappers. But wrappers >>> don't help against every API change… let's say for some reason we'd >>> want to go >>> back to the old char*/int; would wrappers help? no. (Yes, that's maybe a bad >>> example, but just trying to show that, while it has maybe a theoretical gain, I >>> don't see a real practical benefit.) zend_string is really a core structure and >>> extremely simple. Please don't trade simplicity for a small other benefit. >>> >>> Also, we still leave zend_string struct exposed (presumably for allowing inlining). >>> At the point where we are now, many extension authors probably already are >>> also accessing zend_string directly. We're going to have a colorful mix of code >>> using the APIs and not using it, which is the worst we can get now. >>> >>> To end up, I very much like unifying prefixes (ZSTR_*), but I strongly dislike >>> hurting readability and consistency so much for such little benefit. >>> >> I can only express my background on accepting this patch. >> >> In first place, the names ZSTR_* are really speaking, like (z)end_(str)ing and the rest >> mirros the name of the actual member. That is better than STR_*. Or like IS_INTERNED vs >> ZSTR_IS_INTERNED, talking about what happens. With the set len function - yep, probably a bit >> misleading. >> >> Thinking also about the new foreach macros for hash or even zend_hash_num_elements symbol >> which has survived from PHP5 among others. Same here, access to one member encapsulated. Though >> direct accesses ->nNumOfElements is still here and there, where it's useful or no care >> taken. Just to name one analogue. A macro doesn't cost anything, while it does its job >> abstracting things. >> >> The PR is closed. It's not about forcing anyone to not to use ->val and alike >> (maybe indeed it is), but about what one calls application programming interface. aka API. ->val >> is not an API. For the outer codes it's probably the own responsibility on using the APIs vs >> direct access. >> >> Regards >> >> Anatol > > They're definitely speaking and as I said "I very much like unifying prefixes". > That's totally fine and not what I'm complaining about. > > Generally, I'm really just disliking ZSTR_LEN, ZSTR_VAL(), zend_string_set_len() and > zend_string_revert_hash(). > > Sure, but who wants to type that? nNumberOfElements? I don't feel like you can remember > that very well. oh… and wait. It's called nNumOfElements… See? That's why we needed a > wrapper here. > > Point here is that it doesn't abstract anything, it just renames it. It costs the > confusion it creates. Should I directly access it or not? There it was directly accessed, but there > not. If we'd done that, we should have done that at the very beginning IMHO. Not now. > > And still, using a struct is an API. The members have a well defined meaning what they do. The > struct itself is an API, it is an interface giving you the offset from base zend_string * address. > Using "my_len = *((size_t*)((char *)zend_string) + 16))" would be an example of not > programming to the API. That's what a struct provides. A layer of abstraction over the offset. > Hence it's already a level of abstraction. > > I'd like to urge you to not have two (used) APIs. And considering, that IMHO the direct > accessors are much more readable than the macros and currently also much more widely used, I'd > like to remove these 4 macros/inline functions. > > Thanks, > Bob > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > I’m very much in agreement with Bob on the confusion that these macros create. Most of the source still uses zend_string->val or zend_string->len, while now a few places now use ZSTR_VAL(zend_string) and ZSTR_LEN(zend_string). Which am I suppose to use? Do you intend to change every occurrence of direct access to the struct members to use the macros instead? Regards, Aaron Piotrowski