> De : Aaron Piotrowski [mailto:aa...@icicle.io] > Envoyé : mardi 30 juin 2015 04:16 > À : Bob Weinand; Anatol Belski > Cc : internals@lists.php.net > Objet : Re: [PHP-DEV] Improved zend_string API > > > > 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?
I agree that it is confusing and should have been done before, but we chose to introduce the API in 7.0, so that developers can start using it as soon as possible. > Do you intend to > change every occurrence of direct access to the struct members to use the > macros instead? Yes, of course. Keeping a mix would the worst. The objective is to make the whole compliant with the new API. It just required more time than I had before 7.0. So, we proceed in 2 steps : first provide an API, then fix the whole code to use it. In parallel, communicate with extension developers to discourage direct access to the zend_string structure elements. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
