Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:76828
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.43 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53F7F6ED.1050609@lerdorf.com>
References: <CAF+90c_AMJ3SLyX7eDQq-gwFLOCQUfLus+qFfJgAgTzBpZpfEg@mail.gmail.com>
	<D19D208D-06E9-4310-92DD-6F02C666138C@ajf.me>
	<CAEZPtU7yMbMSoB2imCH8NX7nCwXttpnf878vJeBK2UypJ_QxVQ@mail.gmail.com>
	<53F7F6ED.1050609@lerdorf.com>
Date: Sat, 23 Aug 2014 07:46:03 +0200
Message-ID: <CAEZPtU5+UYNJcuAnQ+ZS-U4POweEj_J=XMiP8t4zeo7jbZ_15A@mail.gmail.com>
To: Rasmus Lerdorf <rasmus@lerdorf.com>
Cc: PHP internals <internals@lists.php.net>, Nikita Popov <nikita.ppv@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [RFC] Better type names for int64 RFC
From: pierre.php@gmail.com (Pierre Joye)

hi Rasmus,

On Sat, Aug 23, 2014 at 4:05 AM, Rasmus Lerdorf <rasmus@lerdorf.com> wrote:
> On 8/22/14, 11:38 AM, Pierre Joye wrote:
>> On Aug 22, 2014 5:33 PM, "Andrea Faulds" <ajf@ajf.me> wrote:
>>>
>>>
>>> On 22 Aug 2014, at 12:16, Nikita Popov <nikita.ppv@gmail.com> wrote:
>>>
>>>> As we were not given a chance to resolve this issue before the merge, =
a
>>>> short proposal has been created, which aims to revert all unnecessary
>>>> naming changes and instead use type names that are consistent with the
>>>> existing code base and expectations:
>>>>
>>>>    https://wiki.php.net/rfc/better_type_names_for_int64
>>>>
>>>> Due to the unexpected merge on short notice, with no chance for
>> discussion,
>>>> this issue is blocking a number of other patches. As such I ask if we
>> can
>>>> move through this RFC with a shortened cycle. I would not appreciate
>> having
>>>> to wait three weeks to have a workable codebase again.
>>>
>>> I=E2=80=99m very much in favour of this RFC. It is not necessary to cha=
nge the
>> type names after all; if people turn on compiler warnings, they=E2=80=99=
ll find out
>> what=E2=80=99s broken anyway.
>>
>> That's one of my points. They won't for all cases. And why good names
>> reflecting the underlying type will help. It is what the 1st version if =
the
>> int64 rfc did.
>
> The end result here is likely that in order to write a portable
> extension, extension authors will simply do:
>
> #ifndef IS_LONG
> # define IS_LONG IS_INT
> # define Z_STRLEN Z_STRSIZE
> # define RETURN_LONG RETURN_INT
> # define RETVAL_LONG RETVAL_INT
> # define Z_LVAL Z_IVAL
> ...
> #endif
>
> and not change a single macro anywhere in the actual code.

If the int64 RFC was accepted for 5.x, this solution would be perfect
(except from a confusing usage and type point of view). But you would
also need #ifdef for the declarations when used with zpp, or storing
values in variables, for the zval integer and strings values:

/* for zval string */
#if PHP_MAJOR_VERSION > 6
size_t str_size;
char *str;
#else
int str_size;
char *str;
#endif

Same for zpp argument_

#if PHP_MAJOR_VERSION > 6
zend_string path;
#else
int str_size;
char     *path;
int       path_len;
#endif

Besides that many changes can't be aliased, same APIs or macros use
different arguments or argument content, f.e. the hash api:

f.e.:
- warning at build time:
ex 1:
- php_basename(path_cleaned, path_cleaned_len, NULL, 0,
&file_basename, (size_t *)&file_basename_len TSRMLS_CC);
+  file_basename =3D php_basename(path_cleaned, path_cleaned_len, NULL,
0 TSRMLS_CC);

ex 2:
- add_next_index_string(return_value, fullpath, 1);
+ add_next_index_string(return_value, fullpath);

- or even more dangerous if forgotten (no compile time detection (size
and return value):
ex 1 (hash functions):
- if (zend_hash_find(HASH_OF(options), "add_path", sizeof("add_path"),
(void **)&option) =3D=3D SUCCESS) {
+ if ((option =3D zend_hash_str_find(HASH_OF(options), "add_path",
sizeof("add_path") - 1)) !=3D NULL) {

-  zend_hash_add(prop_handler, name, strlen(name)+1, &hnd,
sizeof(zip_prop_handler), NULL);
+  zend_hash_str_add_mem(prop_handler, name, strlen(name), &hnd,
sizeof(zip_prop_handler));


ex 2:
use of string will need different free calls:
- efree(file_basename);
+ STR_RELEASE(file_basename);



> That's certainly what I would do. And if the current naming survives we
> should provide a macro_compat.h file with the complete set.


The more exts I work on the less I think our current flow for git will
work out of the box. I am slowly considering creating separate
branches for 7 and 5.x, with different packages for the releases
(avoiding to have to upload/download two code bases when only one is
used).  But this is almost another topic, I will post a mail about it
and see how Pickle could help here.

> Overall I don't think trying to make the macro names match the
> underlying types exactly is worth the hassle here. Our audience for this
> is extension authors. In most cases the author doesn't necessarily care
> about the subtle differences between calling it IS_LONG and IS_INT and I
> don't think it actually makes it any more clear to a C developer. "int"
> is not a well-defined cross-platform type anymore than a "long" is.

Well, in the context of of the integer case, zend_uint vs zend_int_t,
it is. Or it is for the case used in the engine. Nikita and I had a
long discussions last night. We came to two conclusions:

- zend_uint should go away.
- uint32_t should be used instead
- zend_int_t could remain
  . it matches the respective, architecture specific int32_t and
int64_t being used behind it. _t standing for _typed just like in the
standard types intXX_t
- We did not reach an agreement about IS_LONG.

My point: If we keep it, it increases the porting bugs. As we already
have seen in many extensions, is confusing and devs forgot to check
the type. IS_INT matches PHP's integer, which is architecture
dependent, devs may be more careful.
Nikita's point is about not changing it for changing it along what is
described in the RFC (@Nikita, please complete :)

Pretty much the same applies to the Z_STRLEN vs T_STRSIZE. Here I came
back to an initial proposal, using Z_STRSIZET or Z_STRSIZE_T.

> So
> we are swapping one set of vague names for another set of vague names.
> I'd much rather see a nice clear README.TYPES or perhaps it is simply
> part of the existing README.PARAMETER_PARSING_API which defines exactly
> what these various macros mean.

No matter the outcome of this discussion, these documents must be
updated (they are not, partially for int64 and almost not for NG).

In other words, it would be nice to see more developers actually
porting extensions to realize the amount of changes are introduced by
NG and by the int64. The sooner is in order of magnitude must larger.
It is not a bad comment, only a fact. Given that, before we choose to
say that it is fine for one part to change APIs/Macros signatures or
names and not for another, we should really get a better view of what
has actually changed. And how we can deal with our old habit to
maintain one tree for many major PHP versions. For many extensions, I
do not think it will be possible, or with unreadable code with 2-3x
more #ifdef all over the place.


Cheers,
--=20
Pierre

@pierrejoye | http://www.libgd.org