Proposal: Introduce a new macro:php_error_docref_ex()

10 years ago by reeze — view source — reply

unread

Hi all,
There are a lot of code use php_error_docref() macro, the first
parameter
mostly is NULL, before PHP7, it looks like:

php_error_docref(NULL TSRML, E_WARNING, "recursion detected");
in PHP7
php_error_docref(NULL, E_WARNING, "recursion detected");
looks better, but the first parameter look dumb.

I did a simple statics, there are:

Null docref: 2530
Not Null docref: 51

I searched on Github, it seems that almost all of the extension use NULL
docref.

So I propose add a new macro, like: php_error_error_docref_ex(E_WARNING,
"xxx").
this could make code looks better and the extension maintainer's easier.

Another option would be just update the php_error_docref() macro to remove
the docref parameter, default to NULL but not add a new macro.

What do you think about it?

[1] https://github.com/php/php-src/pull/1075

--
Reeze Xia
http://reeze.cn

10 years ago by Xinchen Hui — view source — reply

unread

Hi all,
There are a lot of code use php_error_docref() macro, the first
parameter
mostly is NULL, before PHP7, it looks like:

php_error_docref(NULL TSRML, E_WARNING, "recursion detected");
in PHP7
php_error_docref(NULL, E_WARNING, "recursion detected");
looks better, but the first parameter look dumb.

I did a simple statics, there are:

Null docref: 2530
Not Null docref: 51

I searched on Github, it seems that almost all of the extension use NULL
docref.

So I propose add a new macro, like: php_error_error_docref_ex(E_WARNING,
"xxx").
this could make code looks better and the extension maintainer's easier.

Another option would be just update the php_error_docref() macro to remove
the docref parameter, default to NULL but not add a new macro.

What do you think about it?
No.

I can't see any benefit from it, but lots of changes.

everything works well now.

thanks

[1] https://github.com/php/php-src/pull/1075

--
Reeze Xia
http://reeze.cn

--
Xinchen Hui
@Laruence
http://www.laruence.com/

10 years ago by reeze — view source — reply

unread

I think it is a cleanup, it works but there are too many duplication. it is
the time to make the code clean and easier to understand.

I myself don't know why should I need the first NULL when I am a newcomer
of writing extensions, it just seems everyone do that,
before PHP7 there is a strange TSRML_CC macro, it is hard to maintain.

Hi all,
There are a lot of code use php_error_docref() macro, the first
parameter
mostly is NULL, before PHP7, it looks like:

php_error_docref(NULL TSRML, E_WARNING, "recursion detected");
in PHP7
php_error_docref(NULL, E_WARNING, "recursion detected");
looks better, but the first parameter look dumb.

I did a simple statics, there are:

Null docref: 2530
Not Null docref: 51

I searched on Github, it seems that almost all of the extension use NULL
docref.

So I propose add a new macro, like: php_error_error_docref_ex(E_WARNING,
"xxx").
this could make code looks better and the extension maintainer's easier.

Another option would be just update the php_error_docref() macro to
remove
the docref parameter, default to NULL but not add a new macro.

What do you think about it?
No.

I can't see any benefit from it, but lots of changes.

everything works well now.

thanks

[1] https://github.com/php/php-src/pull/1075

--
Reeze Xia
http://reeze.cn

--
Xinchen Hui
@Laruence
http://www.laruence.com/

--
Reeze Xia
http://reeze.cn

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

I think it is a cleanup, it works but there are too many duplication. it
is the time to make the code clean and easier to understand.

I've never used other than NULL also.
Everyone is going to remove TSRM macros, it's right time to clean up.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Xinchen Hui — view source — reply

unread

Hey:

Hi all,

I think it is a cleanup, it works but there are too many duplication. it
is the time to make the code clean and easier to understand.

I've never used other than NULL also.
Everyone is going to remove TSRM macros, it's right time to clean up.
fine, if you want to change..

please, _ex means extending,

so the name should not be php_error_docref_ex...

thanks

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--
Xinchen Hui
@Laruence
http://www.laruence.com/

10 years ago by Dmitry Stogov — view source — reply

unread

Hi,

I don't think it'll improve something, and may complicate merging from PHP5
(this is not a big issue, because merging is already not simple).

In any case, the names should be swapped, to be used as:

php_error_docref(E_WARNING, "recursion detected");

php_error_docref_ex(NULL, E_WARNING, "recursion detected");

also macros with VA_ARGS may be not portable, s it should be
implemented differently.

Thanks. Dmitry.

Hey:

Hi all,

I think it is a cleanup, it works but there are too many duplication. it
is the time to make the code clean and easier to understand.

I've never used other than NULL also.
Everyone is going to remove TSRM macros, it's right time to clean up.
fine, if you want to change..

please, _ex means extending,

so the name should not be php_error_docref_ex...

thanks

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--
Xinchen Hui
@Laruence
http://www.laruence.com/

10 years ago by reeze — view source — reply

unread

Hi,

> Hi,
>
> I don't think it'll improve something, and may complicate merging from
> PHP5 (this is not a big issue, because merging is already not simple).
>
>
Yes it just makes code looks a little better and cleaner.

I didn't realized the merging problem. that might affect diffs which
changes php_error_docref()

> In any case, the names should be swapped, to be used as:
>
> php_error_docref(E_WARNING, "recursion detected");
>
> or
>
> php_error_docref_ex(NULL, E_WARNING, "recursion detected");
>

Yes I prefer this too.

But considering the merging problem, I think I should hold this idea of
update all of those calls.

We may provider a simple version with default null docref for external
extesions, but I didn't have proper name for that.

>
>
> also macros with __VA_ARGS__ may be not portable, s it should be
> implemented differently.
>

Yes I should use ##__VA_ARGS__ instead.

> Thanks. Dmitry.
>
>
>
>
>
>> Hey:
>>
>>
>> > Hi all,
>> >
>> >
>> >>
>> >> I think it is a cleanup, it works but there are too many duplication.
>> it
>> >> is the time to make the code clean and easier to understand.
>> >
>> >
>> > I've never used other than `NULL` also.
>> > Everyone is going to remove TSRM macros, it's right time to clean up.
>> fine, if you want to change..
>>
>> please, _ex means extending,
>>
>> so the name should not be php_error_docref_ex...
>>
>> thanks
>> >
>> > Regards,
>> >
>> > --
>> > Yasuo Ohgaki
>> > yohgaki@ohgaki.net
>>
>>
>>
>> --
>> Xinchen Hui
>> @Laruence
>> http://www.laruence.com/
>>
>
>

--
Reeze Xia
http://reeze.cn

10 years ago by Michael Wallner — view source — reply

unread

Why not leave it as it is? I mean, really, those five strokes?

Wherever it's going, I'm -1 on changing the meaning of the existing macro.

> Hi,
>
>
>
> > Hi,
> >
> > I don't think it'll improve something, and may complicate merging from
> > PHP5 (this is not a big issue, because merging is already not simple).
> >
> >
> Yes it just makes code looks a little better and cleaner.
>
> I didn't realized the merging problem. that might affect diffs which
> changes php_error_docref()
>
>
> > In any case, the names should be swapped, to be used as:
> >
> > php_error_docref(E_WARNING, "recursion detected");
> >
> > or
> >
> > php_error_docref_ex(NULL, E_WARNING, "recursion detected");
> >
>
> Yes I prefer this too.
>
> But considering the merging problem, I think I should hold this idea of
> update all of those calls.
>
> We may provider a simple version with default null docref for external
> extesions, but I didn't have proper name for that.
>
>
> >
> >
> > also macros with __VA_ARGS__ may be not portable, s it should be
> > implemented differently.
> >
>
> Yes I should use ##__VA_ARGS__ instead.
>
>
> > Thanks. Dmitry.
> >
> >
> >
> >
> >
> >> Hey:
> >>
> >> On Wed, Feb 11, 2015 at 4:35 PM, Yasuo Ohgaki
> wrote:
> >> > Hi all,
> >> >
> >> >
> >> >>
> >> >> I think it is a cleanup, it works but there are too many duplication.
> >> it
> >> >> is the time to make the code clean and easier to understand.
> >> >
> >> >
> >> > I've never used other than `NULL` also.
> >> > Everyone is going to remove TSRM macros, it's right time to clean up.
> >> fine, if you want to change..
> >>
> >> please, _ex means extending,
> >>
> >> so the name should not be php_error_docref_ex...
> >>
> >> thanks
> >> >
> >> > Regards,
> >> >
> >> > --
> >> > Yasuo Ohgaki
> >> > yohgaki@ohgaki.net
> >>
> >>
> >>
> >> --
> >> Xinchen Hui
> >> @Laruence
> >> http://www.laruence.com/
> >>
> >
> >
>
>
> --
> Reeze Xia
> http://reeze.cn

10 years ago by Dmitry Stogov — view source — reply

unread

We don't use macros with variable number of arguments in PHP.
this is not portable.

Thanks. Dmitry.

> Hi,
>
>
>
>> Hi,
>>
>> I don't think it'll improve something, and may complicate merging from
>> PHP5 (this is not a big issue, because merging is already not simple).
>>
>>
> Yes it just makes code looks a little better and cleaner.
>
> I didn't realized the merging problem. that might affect diffs which
> changes php_error_docref()
>
>
>> In any case, the names should be swapped, to be used as:
>>
>> php_error_docref(E_WARNING, "recursion detected");
>>
>> or
>>
>> php_error_docref_ex(NULL, E_WARNING, "recursion detected");
>>
>
> Yes I prefer this too.
>
> But considering the merging problem, I think I should hold this idea of
> update all of those calls.
>
> We may provider a simple version with default null docref for external
> extesions, but I didn't have proper name for that.
>
>
>>
>>
>> also macros with __VA_ARGS__ may be not portable, s it should be
>> implemented differently.
>>
>
> Yes I should use ##__VA_ARGS__ instead.
>
>
>> Thanks. Dmitry.
>>
>>
>>
>>
>>
>>> Hey:
>>>
>>> On Wed, Feb 11, 2015 at 4:35 PM, Yasuo Ohgaki
>>> wrote:
>>> > Hi all,
>>> >
>>> >
>>> >>
>>> >> I think it is a cleanup, it works but there are too many duplication.
>>> it
>>> >> is the time to make the code clean and easier to understand.
>>> >
>>> >
>>> > I've never used other than `NULL` also.
>>> > Everyone is going to remove TSRM macros, it's right time to clean up.
>>> fine, if you want to change..
>>>
>>> please, _ex means extending,
>>>
>>> so the name should not be php_error_docref_ex...
>>>
>>> thanks
>>> >
>>> > Regards,
>>> >
>>> > --
>>> > Yasuo Ohgaki
>>> > yohgaki@ohgaki.net
>>>
>>>
>>>
>>> --
>>> Xinchen Hui
>>> @Laruence
>>> http://www.laruence.com/
>>>
>>
>>
>
>
> --
> Reeze Xia
> http://reeze.cn

10 years ago by reeze — view source — reply

unread

Hi,

> We don't use macros with variable number of arguments in PHP.
> this is not portable.
>

We could don't have to use variadic macros, we could try something like:

*+PHPAPI void php_error_doc0(int type, const char *format, ...)*

*+{*

*+* *va_list args;*

*+*

*+* *va_start(args, format);*

*+* *php_error_docref0(NULL, type, format, args);*

*+* *va_end(args);*

*+}*

*+*

*+#define php_error_doc php_error_doc0*

*+*

>
> Thanks. Dmitry.
>
>
>
>> Hi,
>>
>>
>>
>>> Hi,
>>>
>>> I don't think it'll improve something, and may complicate merging from
>>> PHP5 (this is not a big issue, because merging is already not simple).
>>>
>>>
>> Yes it just makes code looks a little better and cleaner.
>>
>> I didn't realized the merging problem. that might affect diffs which
>> changes php_error_docref()
>>
>>
>>> In any case, the names should be swapped, to be used as:
>>>
>>> php_error_docref(E_WARNING, "recursion detected");
>>>
>>> or
>>>
>>> php_error_docref_ex(NULL, E_WARNING, "recursion detected");
>>>
>>
>> Yes I prefer this too.
>>
>> But considering the merging problem, I think I should hold this idea of
>> update all of those calls.
>>
>> We may provider a simple version with default null docref for external
>> extesions, but I didn't have proper name for that.
>>
>>
>>>
>>>
>>> also macros with __VA_ARGS__ may be not portable, s it should be
>>> implemented differently.
>>>
>>
>> Yes I should use ##__VA_ARGS__ instead.
>>
>>
>>> Thanks. Dmitry.
>>>
>>>
>>>
>>>
>>>
>>>> Hey:
>>>>
>>>> On Wed, Feb 11, 2015 at 4:35 PM, Yasuo Ohgaki
>>>> wrote:
>>>> > Hi all,
>>>> >
>>>> >
>>>> >>
>>>> >> I think it is a cleanup, it works but there are too many
>>>> duplication. it
>>>> >> is the time to make the code clean and easier to understand.
>>>> >
>>>> >
>>>> > I've never used other than `NULL` also.
>>>> > Everyone is going to remove TSRM macros, it's right time to clean up.
>>>> fine, if you want to change..
>>>>
>>>> please, _ex means extending,
>>>>
>>>> so the name should not be php_error_docref_ex...
>>>>
>>>> thanks
>>>> >
>>>> > Regards,
>>>> >
>>>> > --
>>>> > Yasuo Ohgaki
>>>> > yohgaki@ohgaki.net
>>>>
>>>>
>>>>
>>>> --
>>>> Xinchen Hui
>>>> @Laruence
>>>> http://www.laruence.com/
>>>>
>>>
>>>
>>
>>
>> --
>> Reeze Xia
>> http://reeze.cn
>>
>
>

--
Reeze Xia
http://reeze.cn

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

I don't think it'll improve something, and may complicate merging from
PHP5 (this is not a big issue, because merging is already not simple).

In any case, the names should be swapped, to be used as:
php_error_docref(E_WARNING, "recursion detected");
or
php_error_docref_ex(NULL, E_WARNING, "recursion detected");
also macros with VA_ARGS may be not portable, s it should be
implemented differently.

It's even better.
+1

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by reeze — view source — reply

unread

PS:

Github search result:
https://github.com/search?l=c&q=php_error_docref&ref=searchresults&type=Code&utf8=%E2%9C%93

It seems that no one need the docref at all.

Hi all,
There are a lot of code use php_error_docref() macro, the first
parameter
mostly is NULL, before PHP7, it looks like:

php_error_docref(NULL TSRML, E_WARNING, "recursion detected");
in PHP7
php_error_docref(NULL, E_WARNING, "recursion detected");
looks better, but the first parameter look dumb.

I did a simple statics, there are:

Null docref: 2530
Not Null docref: 51

I searched on Github, it seems that almost all of the extension use NULL
docref.

So I propose add a new macro, like: php_error_error_docref_ex(E_WARNING,
"xxx").
this could make code looks better and the extension maintainer's easier.

Another option would be just update the php_error_docref() macro to remove
the docref parameter, default to NULL but not add a new macro.

What do you think about it?

[1] https://github.com/php/php-src/pull/1075

--
Reeze Xia
http://reeze.cn

--
Reeze Xia
http://reeze.cn