> On Jun 29, 2015, at 7:37 PM, Bob Weinand <bobw...@hotmail.com> wrote: > > Hey Anatol, > >> Am 29.06.2015 um 22:41 schrieb Anatol Belski <anatol....@belski.net>: >> >> Hi Bob, >> >>> -----Original Message----- >>> From: Bob Weinand [mailto:bobw...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
