Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73965
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
Message-ID: <8b32b5b5b6d14c0c7aff1cb52087820f.squirrel@webmail.klapt.com>
In-Reply-To:
 <alpine.DEB.2.10.1405060956350.4404@whisky.home.derickrethans.nl>
References: <a1b3a511b2e0163fd87a6ab639c52817.squirrel@webmail.klapt.com>
    <alpine.DEB.2.10.1405060956350.4404@whisky.home.derickrethans.nl>
Date: Tue, 6 May 2014 12:58:54 +0200
To: "Derick Rethans" <derick@php.net>
Cc: "Anatol Belski" <anatol.php@belski.net>,
 "PHP Internals" <internals@lists.php.net>
User-Agent: SquirrelMail/1.5.2 [SVN]
MIME-Version: 1.0
Content-Type: text/plain;charset=UTF-8
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [RFC] 64 bit platform improvements for string length
 and integer
From: anatol.php@belski.net ("Anatol Belski")

Hi Derick,

On Tue, May 6, 2014 11:13, Derick Rethans wrote:
> On Tue, 6 May 2014, Anatol Belski wrote:
>
>
>> I would sincerely invite everyone to the resurrected
>> str_size_and_int64 RFC discussion.
>>
>> I propose the discussion to last for one week as allowed by the voting
>> RFC because this topic has already been discussed to death previously
>> (any objections?). As no userland change is introduced, the discussion
>> would end and the voting would start on May 13th with 50%+1 votes
>> requirement.
>>
>> https://wiki.php.net/rfc/size_t_and_int64_next
>>
>
> I see this still says "The usage of long datatype continues on 32 bit
> platforms." — do I understand correctly that on 32bit platforms, there
> won't still be a 64bit integer in PHP and that this "only" makes 64bit
> integers work on the Windows LLP64 model? Or are they still 32bit integers
> there?
>
Yes, this adds int64 to the windows builds and size_t everywhere and hence
makes it work consistent all 64 bit platforms. Regarding int64 on 32 bit
platform - no, this is not implemented. There are various reasons for
that, the main reason here - the complexity. To the time of starting such
a huge thing the danger it to be not finished is big. You can see this as
a stage which makes int64 on 32 bit platforms at least hypotetically
possible. That might be not visible well to the external APIs, but
internally using int64 on 32 bit platforms would at least double the
complexity of the patch itself (which is already not a light one). Also,
without it we can't move further with overall improvement. And we can't
achieve everything in one single patch, obviously.

> Under "Accepting values with zend_parse_parameters()"
>
>
> “l” 	“i” 	to accept integer argument, the internal var has to be declared
> as php_int_t (inside PHP) or zend_int_t (inside Zend) “L” 	“I” 	to accept
> integer argument with range check, the internal var has to be declared as
> php_int_t (inside PHP) or zend_int_t (inside Zend)
>
> Using l and I is going to be major confusing and incredibly hard to
> spot.
>
> "'l', 'L', 's', 'p' parameter formats aren't available anymore"
>
>
> That means that every zpp call now needs to have an #ifdef around it to
> support post-this-patch and pre-this-patch PHP versions, as well as around
> their variable declarations. I'm afraid that will result in a huge mess,
> so we should look at this again.
>
> Under "Example on accepting parameters with zpp"
>
>
> You have:
> zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "iISP"
>
> But why is both i and I allowed (or did I misread the Old/New before?)
> If so, then my comment on i, l, I is already valid :-)
>
The syntax shown there is pure new syntax. Please take a look at the
migration stuff here
http://git.php.net/?p=php-src.git;a=tree;f=compat;hb=refs/heads/str_size_and_int64
(also mentioned in the RFC). It's relatively fresh yet, but already offers
some automatic and will definitely get better on and on. The compat.h
header defines ZPP_FMT_COMPAT macros, also there's the clang zpp checker
which will reveal any incompatibility. I think you misread the "big I"
with "small l" which can happen also another way round :)


> Under "Example on printf specs usage"
>
>
> You have:
> php_error_docref(NULL TSRMLS_CC, E_WARNING, "Value '" ZEND_INT_FMT "' is
> out of range", i0);
>
> Using that macro there feels really unnatural, and many C developers
> will not bother to figure out that they use that, of course ,because they
> are familiar with "%d" and "%l".
>
> And similarly under "Example on printf specs usage (no BC)" for:
>
>
> "Value '%pd' is out of range", i0
>
>
> The new macros that you use in the example for "char *dup_substr(zval
> *s, zval *i)" also infer that all those accesses need to have a big
> #ifdef around stuff to support multiple versions. Again, making it a
> major pain for extension developers to support pre- and post-patch PHP
> versions.
>
For this two I would mention the migration path as well. ZEND_INT_FMT is
exactly for the purpose to output zend_int_t be it 64 or 32 bit and care
about BC. It will define to the right format for the older PHP through
compat.h. The %d is kept for the case of the pure 32 bit int usage.
Previously it is %ld but it couldn't work anymore in the new code as it
would mean different things with consistent int64 support. The %pd variant
however can be used in the developments without need of BC.

With the new macros names - they're all aliased in the compat.h for the
older versions. So the general idea of the migration path is the usage of
the new clean syntax for the development (like calling a cat a cat) and
retaining the compat layer to the older versions. Once migrated, that can
be carried on.

> I'm afraid with all those concerns, I will have to vote "no".
>
Of course that would make me sad. Not only for the work done on that, but
also for the future perspectives and for the quantity of the users who
wish such thing.

Cheers

Anatol