64-bit integers and 64-bit string length patch is ready to be merged

11 years ago by Andrea Faulds — view source

unread

The only thing that I don't like is a massive renaming described here
https://wiki.php.net/rfc/size_t_and_int64_next#semantical_macro_renamings

IS_LONG -> IS_INT
Z_LVAL -> L_IVAL
etc

On one hand using INT may be more consistent, on the other hand it's going
to break habits and make addition headache for merging from php-5 (I know,
phpng already made problems)

I don’t like this. With the bigints patch and RFC, I want to keep IS_LONG and the word “long” to distinguish between what would be the two types of integer:

IS_LONG/long - 32-bit or 64-bit integer (machine-dependant)
IS_BIGINT/bigint - arbitrary-size integer
IS_BIGINT_OR_LONG/integer - either a long or a bigint (pseudo-type)

Replacing IS_LONG with IS_INT kinda ruins my naming scheme. The intention is that “integer” and “int” are synonyms for “long or bigint”. However, if internally an int is one thing and to userland it’s another, that would be problematic. If this goes through, I’d probably make my bigints patch rename IS_INT to something new again, probably IS_SMALLINT or even back to IS_LONG.

If it must be renamed, could it be something else? IS_LONGLONG perhaps? Seems stupid on the face of it, but it’s actually a C `long long` underneath, isn’t it? FWIW, I prefer Z_LLVAL to Z_IVAL.

Andrea Faulds
http://ajf.me/

11 years ago by Pierre Joye — view source

unread

The only thing that I don't like is a massive renaming described here

https://wiki.php.net/rfc/size_t_and_int64_next#semantical_macro_renamings

IS_LONG -> IS_INT
Z_LVAL -> L_IVAL
etc

On one hand using INT may be more consistent, on the other hand it's
going
to break habits and make addition headache for merging from php-5 (I
know,
phpng already made problems)

I don’t like this. With the bigints patch and RFC, I want to keep IS_LONG
and the word “long” to distinguish between what would be the two types of
integer:

IS_LONG/long - 32-bit or 64-bit integer (machine-dependant)

IS_BIGINT/bigint - arbitrary-size integer

IS_BIGINT_OR_LONG/integer - either a long or a bigint (pseudo-type)

Replacing IS_LONG with IS_INT kinda ruins my naming scheme. The intention
is that “integer” and “int” are synonyms for “long or bigint”. However, if
internally an int is one thing and to userland it’s another, that would be
problematic. If this goes through, I’d probably make my bigints patch
rename IS_INT to something new again, probably IS_SMALLINT or even back to
IS_LONG.

If it must be renamed, could it be something else? IS_LONGLONG perhaps?
Seems stupid on the face of it, but it’s actually a C long long
underneath, isn’t it? FWIW, I prefer Z_LLVAL to Z_IVAL.

The original patch used the right naming based on the type used behind it.

In any case, the patch represents what the rfc and the discussions around
it say. I rather merge it asal and begin on the (long) list of todos for 7.

If you like to propose new naming with the bigint rfc, please go ahead.
Time is also an issue as ppl are beginning to migrate their extensions to
test. Changing them again takes time.

Cheers,
Pierre

11 years ago by Andrea Faulds — view source

unread

The original patch used the right naming based on the type used behind it.

So IS_LONGLONG, then?

In any case, the patch represents what the rfc and the discussions around
it say. I rather merge it asal and begin on the (long) list of todos for 7.

If you like to propose new naming with the bigint rfc, please go ahead.
Time is also an issue as ppl are beginning to migrate their extensions to
test. Changing them again takes time.

Forgive me if I’m wrong, but wouldn’t changing the name again just be a simple matter of global find/replace?

--
Andrea Faulds
http://ajf.me/

11 years ago by Pierre Joye — view source

unread

The original patch used the right naming based on the type used behind
it.

So IS_LONGLONG, then?

The original patch, the one from last year. We have made compromises to
fulfill other devs requests, three times.

Now to be honest I would merge it right away. I do not see why we need to
discuss that again. When I see how phpng got accepted without a word, time
to stop arguing about such things. Merge and move back to code. If one
thinks that one thing or another should be changed, in this patch or phpng,
he will need to write a rfc (and not in 6 months :).

In any case, the patch represents what the rfc and the discussions
around
it say. I rather merge it asal and begin on the (long) list of todos
for 7.

If you like to propose new naming with the bigint rfc, please go ahead.
Time is also an issue as ppl are beginning to migrate their extensions
to
test. Changing them again takes time.

Forgive me if I’m wrong, but wouldn’t changing the name again just be a
simple matter of global find/replace?

No, totally not :)

Even less as bigint support should be added too. And if bigint support will
be optional, the code will need more changes.

11 years ago by Andrea Faulds — view source

unread

Now to be honest I would merge it right away. I do not see why we need to discuss that again. When I see how phpng got accepted without a word, time to stop arguing about such things. Merge and move back to code. If one thinks that one thing or another should be changed, in this patch or phpng, he will need to write a rfc (and not in 6 months :).

This is a trivial name change. We have only once chance to get it right since the IS_* constants are used so, so widely. We should get it right now.

Forgive me if I’m wrong, but wouldn’t changing the name again just be a simple matter of global find/replace?

No, totally not :)

Why wouldn’t it? Could you elaborate?

--
Andrea Faulds
http://ajf.me/

11 years ago by Pierre Joye — view source

unread

Now to be honest I would merge it right away. I do not see why we need
to discuss that again. When I see how phpng got accepted without a word,
time to stop arguing about such things. Merge and move back to code. If one
thinks that one thing or another should be changed, in this patch or phpng,
he will need to write a rfc (and not in 6 months :).

This is a trivial name change. We have only once chance to get it right
since the IS_* constants are used so, so widely. We should get it right now.

Then please do a rfc, but we are not going to rewamp the patch for the 6th
time. It has been accepted and we have been more than cooperative.

Forgive me if I’m wrong, but wouldn’t changing the name again just be
a simple matter of global find/replace?

No, totally not :)

Why wouldn’t it? Could you elaborate?

I would instead to ask you to try to migrate a not so trivial extension :)

11 years ago by Andrea Faulds — view source

unread

Then please do a rfc, but we are not going to rewamp the patch for the 6th
time. It has been accepted and we have been more than cooperative.

Do we really need to delay this by three weeks?

Forgive me if I’m wrong, but wouldn’t changing the name again just be
a simple matter of global find/replace?

No, totally not :)

Why wouldn’t it? Could you elaborate?

I would instead to ask you to try to migrate a not so trivial extension :)

No, seriously. Why can’t a sed script be used to change a constant name? Can you tell me why that wouldn’t work?

--
Andrea Faulds
http://ajf.me/

11 years ago by Dmitry Stogov — view source

unread

Pierre, wait a day, and if we won't have many developers, who against the
new names - commit it as is.

Thanks. Dmitry.

The original patch used the right naming based on the type used behind
it.

So IS_LONGLONG, then?

The original patch, the one from last year. We have made compromises to
fulfill other devs requests, three times.

Now to be honest I would merge it right away. I do not see why we need to
discuss that again. When I see how phpng got accepted without a word, time
to stop arguing about such things. Merge and move back to code. If one
thinks that one thing or another should be changed, in this patch or phpng,
he will need to write a rfc (and not in 6 months :).

In any case, the patch represents what the rfc and the discussions
around
it say. I rather merge it asal and begin on the (long) list of todos
for 7.

If you like to propose new naming with the bigint rfc, please go ahead.
Time is also an issue as ppl are beginning to migrate their extensions
to
test. Changing them again takes time.

Forgive me if I’m wrong, but wouldn’t changing the name again just be a
simple matter of global find/replace?

No, totally not :)

Even less as bigint support should be added too. And if bigint support
will be optional, the code will need more changes.

11 years ago by Andrea Faulds — view source

unread

Pierre, wait a day, and if we won't have many developers, who against the new names - commit it as is.

Perhaps none will be against it now, however, how will they feel when they have to change IS_INT in their extensions again to a new name bigints would necessarily introduce?

Andrea Faulds
http://ajf.me/

11 years ago by Dmitry Stogov — view source

unread

In worst case, we will always able to make a new RFC and rename them back,
but I really don't like to delay this patch just because few people
(including me) are not agree with names. RFC was already delayed for
months, and actually, it was voted with section about new names.

Thanks. Dmitry.

Pierre, wait a day, and if we won't have many developers, who against
the new names - commit it as is.

Perhaps none will be against it now, however, how will they feel when they
have to change IS_INT in their extensions again to a new name bigints
would necessarily introduce?

Andrea Faulds
http://ajf.me/

11 years ago by Pierre Joye — view source

unread

Pierre, wait a day, and if we won't have many developers, who against the
new names - commit it as is.

Thanks. Dmitry.
We have waited months due to phpng. We have waited months due to objection
after the rfc was accepted already (2nd attempt).

Why should we follow different rules than other RFCs? Bigint is a work in
progress, there is no vote, there is not even a discussion on this list
about it. Asking us to hang on again for yet another rfc is really not
something I can live with.

11 years ago by Andrea Faulds — view source

unread

Why should we follow different rules than other RFCs? Bigint is a work in progress, there is no vote, there is not even a discussion on this list about it. Asking us to hang on again for yet another rfc is really not something I can live with.

There is a discussion on this list, but you have a point.

I’ll consider either creating an RFC to rename it or finding some sort of workaround.

--
Andrea Faulds
http://ajf.me/

11 years ago by Dmitry Stogov — view source

unread

I completely agree.

Dmitry.

Pierre, wait a day, and if we won't have many developers, who against
the new names - commit it as is.

Thanks. Dmitry.
We have waited months due to phpng. We have waited months due to objection
after the rfc was accepted already (2nd attempt).

Why should we follow different rules than other RFCs? Bigint is a work in
progress, there is no vote, there is not even a discussion on this list
about it. Asking us to hang on again for yet another rfc is really not
something I can live with.

11 years ago by Matteo Beccati — view source

unread

IS_LONG/long - 32-bit or 64-bit integer (machine-dependant)

IS_BIGINT/bigint - arbitrary-size integer

IS_BIGINT_OR_LONG/integer - either a long or a bigint (pseudo-type)

Replacing IS_LONG with IS_INT kinda ruins my naming scheme. The intention is that “integer” and “int” are synonyms for “long or bigint”. However, if internally an int is one thing and to userland it’s another, that would be problematic. If this goes through, I’d probably make my bigints patch rename IS_INT to something new again, probably IS_SMALLINT or even back to IS_LONG.

wouldn't the following work for you?

IS_INT
IS_BIGINT
IS_INT_OR_BIGINT

After all, SQL has INT(EGER) and BIGINT, albeit with different meanings.
In fact "bigint" itself to me and possibly many other developers means a
64bit int, not a GMP int.

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

11 years ago by Andrea Faulds — view source

unread

IS_LONG/long - 32-bit or 64-bit integer (machine-dependant)

IS_BIGINT/bigint - arbitrary-size integer

IS_BIGINT_OR_LONG/integer - either a long or a bigint (pseudo-type)

Replacing IS_LONG with IS_INT kinda ruins my naming scheme. The intention is that “integer” and “int” are synonyms for “long or bigint”. However, if internally an int is one thing and to userland it’s another, that would be problematic. If this goes through, I’d probably make my bigints patch rename IS_INT to something new again, probably IS_SMALLINT or even back to IS_LONG.

wouldn't the following work for you?

IS_INT

IS_BIGINT

IS_INT_OR_BIGINT

After all, SQL has INT(EGER) and BIGINT, albeit with different meanings.
In fact "bigint" itself to me and possibly many other developers means a
64bit int, not a GMP int.

It’s doable, it’s just confusing. From userland, “int” can be either IS_INT or IS_BIGINT. I’d rather it was IS_LONG or something.

--
Andrea Faulds
http://ajf.me/

11 years ago by christopher jones — view source

unread

Hi,

Thanks to Anatol and Pierre the 64-bit patch is ready
https://github.com/weltling/php-src

I made quick code review and don't see any technical problems now.

The performance and memory consumption difference is negligible. see
https://docs.google.com/spreadsheets/d/1PD4oiiXz6B0JbeZYnUSat5fHoq3_jAiCeI2cGHJ3UtQ/edit#gid=0

The patch breaks one test on 32-bit Linux:
ext/date/tests/bug53437_var3.phpt (seems to be a bogus test and wrong
behavior in php5.6 and below) and one test on 64-bit Linux:
ext/standard/tests/array/array_pad_variation2.phpt (has to be analyzed)

The only thing that I don't like is a massive renaming described here
https://wiki.php.net/rfc/size_t_and_int64_next#semantical_macro_renamings

IS_LONG -> IS_INT
Z_LVAL -> L_IVAL
etc

On one hand using INT may be more consistent, on the other hand it's going
to break habits and make addition headache for merging from php-5 (I know,
phpng already made problems)

I'm not sure how to proceed. If I'm alone, lets go ahead with new names. If
some others prefer old names we will probably need voting.

Despite of renaming, I would like to see this patch in master ASAP.

Thanks. Dmitry.

We have the code ready to go. The change was voted OK. I say merge it.

Chris

--
christopher.jones@oracle.com http://twitter.com/ghrd
Free PHP & Oracle book:
http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html

11 years ago by Nikita Popov — view source

unread

Hi,

Thanks to Anatol and Pierre the 64-bit patch is ready
https://github.com/weltling/php-src

I made quick code review and don't see any technical problems now.

The performance and memory consumption difference is negligible. see

https://docs.google.com/spreadsheets/d/1PD4oiiXz6B0JbeZYnUSat5fHoq3_jAiCeI2cGHJ3UtQ/edit#gid=0

The patch breaks one test on 32-bit Linux:
ext/date/tests/bug53437_var3.phpt (seems to be a bogus test and wrong
behavior in php5.6 and below) and one test on 64-bit Linux:
ext/standard/tests/array/array_pad_variation2.phpt (has to be analyzed)

The only thing that I don't like is a massive renaming described here
https://wiki.php.net/rfc/size_t_and_int64_next#semantical_macro_renamings

IS_LONG -> IS_INT
Z_LVAL -> L_IVAL
etc

On one hand using INT may be more consistent, on the other hand it's going
to break habits and make addition headache for merging from php-5 (I know,
phpng already made problems)

I'm not sure how to proceed. If I'm alone, lets go ahead with new names. If
some others prefer old names we will probably need voting.

Despite of renaming, I would like to see this patch in master ASAP.

Thanks. Dmitry.

I am against merging this with the long->int rename everywhere. This seems
like change for the sake of change.

I am also concerned that we now have zend_uint_t (a 64-bit integer type)
and zend_uint (a 32-bit integer type). Notice the difference? Yes, it's the
missing _t.

I would appreciate it if we could consider the following naming convention:

zend_(u)int - 32 bit integer type
zend_(u)long - 64 bit integer type (on 64 bit systems)

This retains the original meaning of the type, with the tweak that
zend_(u)long will be 64bit on LLP64 systems as well. This avoids the
confusion of having two types that only differ by a _t suffix and have
totally different meanings. It also removes any need to rename everything
from LONG to INT.

Thanks,
Nikita

11 years ago by Andrea Faulds — view source

unread

I am also concerned that we now have zend_uint_t (a 64-bit integer type)
and zend_uint (a 32-bit integer type). Notice the difference? Yes, it's the
missing _t.

I would appreciate it if we could consider the following naming convention:

zend_(u)int - 32 bit integer type

zend_(u)long - 64 bit integer type (on 64 bit systems)

This retains the original meaning of the type, with the tweak that
zend_(u)long will be 64bit on LLP64 systems as well. This avoids the
confusion of having two types that only differ by a _t suffix and have
totally different meanings. It also removes any need to rename everything
from LONG to INT.

I was wondering if zend_uint was still 32-bit like I thought it was. I guess I mistook zend_uint_t for zend_uint, which backs up your (implied?) point about confusion.

Your proposal sounds like a great idea, I’d be very in favour of this.

--
Andrea Faulds
http://ajf.me/

11 years ago by Pierre Joye — view source

unread

I am against merging this with the long->int rename everywhere. This
seems like change for the sake of change.

It is accepted and ready to be merged.

I am also concerned that we now have zend_uint_t (a 64-bit integer type)
and zend_uint (a 32-bit integer type). Notice the difference? Yes, it's the
missing _t.

I would appreciate it if we could consider the following naming
convention:

zend_(u)int - 32 bit integer type

zend_(u)long - 64 bit integer type (on 64 bit systems)

The original patch used int32 and int64, _t. Was rejected because too much
changes. I was even more strict in the very first version by using only
stdint types and no alias or random names. Rejected as well.

Long means absolutely nothing in term of integer size. We should ban its
usage. So I am against anything using long.

That being said:

patch will be merged. Most likely tomorrow
if minor tweaks like the uint one you refer are necessary, and that one
makes sense, it can be done later

Now, I asked everyone to stop applying or trying to apply double standards.
Thanks.

Cheers,
Pierre

11 years ago by Dmitry Stogov — view source

unread

zend_long/zend_ulong without renaming everything else would be a perfect
solution from my point of view.

Thanks. Dmitry.

Hi,

Thanks to Anatol and Pierre the 64-bit patch is ready
https://github.com/weltling/php-src

I made quick code review and don't see any technical problems now.

The performance and memory consumption difference is negligible. see

https://docs.google.com/spreadsheets/d/1PD4oiiXz6B0JbeZYnUSat5fHoq3_jAiCeI2cGHJ3UtQ/edit#gid=0

The patch breaks one test on 32-bit Linux:
ext/date/tests/bug53437_var3.phpt (seems to be a bogus test and wrong
behavior in php5.6 and below) and one test on 64-bit Linux:
ext/standard/tests/array/array_pad_variation2.phpt (has to be analyzed)

The only thing that I don't like is a massive renaming described here
https://wiki.php.net/rfc/size_t_and_int64_next#semantical_macro_renamings

IS_LONG -> IS_INT
Z_LVAL -> L_IVAL
etc

On one hand using INT may be more consistent, on the other hand it's going
to break habits and make addition headache for merging from php-5 (I know,
phpng already made problems)

I'm not sure how to proceed. If I'm alone, lets go ahead with new names.
If
some others prefer old names we will probably need voting.

Despite of renaming, I would like to see this patch in master ASAP.

Thanks. Dmitry.

I am against merging this with the long->int rename everywhere. This seems
like change for the sake of change.

I am also concerned that we now have zend_uint_t (a 64-bit integer type)
and zend_uint (a 32-bit integer type). Notice the difference? Yes, it's the
missing _t.

I would appreciate it if we could consider the following naming convention:

zend_(u)int - 32 bit integer type

zend_(u)long - 64 bit integer type (on 64 bit systems)

This retains the original meaning of the type, with the tweak that
zend_(u)long will be 64bit on LLP64 systems as well. This avoids the
confusion of having two types that only differ by a _t suffix and have
totally different meanings. It also removes any need to rename everything
from LONG to INT.

Thanks,
Nikita

11 years ago by Pierre Joye — view source

unread

zend_long/zend_ulong without renaming everything else would be a perfect
solution from my point of view.

Again, no. long is the worst type ever, be as type or in names (ppl
then use this type to match the macro names).

If there is a need for clearer names, we can deal with that later.
Maybe Andrea will do it with her bigint patch. But using names that do
not actually represent what they actually use is a no go. By the way,
we have the same issue for the zend_string macros, just damned
confusing and the amount of fixes about their usage confirm that.

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

11 years ago by Anatol Belski — view source

unread

Moin,

Hi,

Thanks to Anatol and Pierre the 64-bit patch is ready
https://github.com/weltling/php-src

I made quick code review and don't see any technical problems now.

The performance and memory consumption difference is negligible. see
https://docs.google.com/spreadsheets/d/1PD4oiiXz6B0JbeZYnUSat5fHoq3_jAiCeI
2cGHJ3UtQ/edit#gid=0

The patch breaks one test on 32-bit Linux:
ext/date/tests/bug53437_var3.phpt (seems to be a bogus test and wrong
behavior in php5.6 and below) and one test on 64-bit Linux:
ext/standard/tests/array/array_pad_variation2.phpt (has to be analyzed)

The only thing that I don't like is a massive renaming described here
https://wiki.php.net/rfc/size_t_and_int64_next#semantical_macro_renamings

IS_LONG -> IS_INT
Z_LVAL -> L_IVAL
etc

On one hand using INT may be more consistent, on the other hand it's
going to break habits and make addition headache for merging from php-5 (I
know, phpng already made problems)

I'm not sure how to proceed. If I'm alone, lets go ahead with new names.
If
some others prefer old names we will probably need voting.

Despite of renaming, I would like to see this patch in master ASAP.

Thanks. Dmitry.

the patch is merged by now as is. I greatefuly thank all the supporters,
especially Nikita for the idea on how to join phpng and str-size_and_int64
with minimal loss and Dmitry for the closer look. I think it's feasible
solving the naming issues with Andrea's or some separate RFC.

Regards

anatol

64-bit integers and 64-bit string length patch is ready to be merged

If it must be renamed, could it be something else? IS_LONGLONG perhaps? Seems stupid on the face of it, but it’s actually a C long long underneath, isn’t it? FWIW, I prefer Z_LLVAL to Z_IVAL.

Perhaps none will be against it now, however, how will they feel when they have to change IS_INT in their extensions again to a new name bigints would necessarily introduce?

Perhaps none will be against it now, however, how will they feel when they have to change IS_INT in their extensions again to a new name bigints would necessarily introduce?

Cheers

Cheers,

If it must be renamed, could it be something else? IS_LONGLONG perhaps? Seems stupid on the face of it, but it’s actually a C `long long` underneath, isn’t it? FWIW, I prefer Z_LLVAL to Z_IVAL.

Perhaps none will be against it now, however, how will they feel when they
have to change IS_INT in their extensions again to a new name bigints
would necessarily introduce?