Hi Sara,
https://gist.github.com/dstogov/6a90601872b538d2ddd6
I see no problems committing this (running tests now).
Thanks. Dmitry.
On Wed, Jun 10, 2015 at 9:02 PM, Dmitry Stogov <dmi...@zend.com> wrote:
>
>
> On Wed, Jun 10, 2015 at 6:00 PM, Sara Golemon <poll...@php.net> wrote:
>
>> On Wed, Jun 10, 2015 at 7:50 AM, Sara Golemon <poll...@php.net> wrote:
>> > On Wed, Jun 10, 2015 at 7:42 AM, Sara Golemon <poll...@php.net> wrote:
>> >> Dmitry, what's the reasoning behind this diff in the first place?
>> >> Doesn't the compiler fold (<const-string> . <const-string>) already
>> >> anyhow? How would we wind up with CONCAT_CONST_CONST at runtime?
>> >>
>> > Derp. Looked again, and it's (const *OR* string) && (const *OR*
>> > string). I read those as ANDs initially.
>> >
>> > But now I'm confused again, because that makes it look like we can
>> > reach this with an expression like "1 . 2" 1 and 2 are consts, but not
>> > strings, so then we get to:
>> >
>> > zend_string *op1_str = Z_STR_P(op1);
>> >
>> > Which will lead to op1_str pointing at 0x00000001 and a segfault.
>> >
>> > What am I missing here?
>> >
>> Okay, answered my own question (I need to be gentler with the [Send]
>> button).
>>
>> Zend/zend_compile.c sets up an assumption:
>>
>> if (left_node.op_type == IS_CONST) {
>> convert_to_string(&left_node.u.constant);
>> }
>> if (right_node.op_type == IS_CONST) {
>> convert_to_string(&right_node.u.constant);
>> }
>>
>> So any const nodes passed to CONCAT will always be strings (having
>> been preconverted), and the if check using an OR make sense, because
>> if it's CONST, then we KNOW it's a string, and there's no point
>> testing for it. We won't ever reach CONCAT_CONST_CONST (since that
>> would be folded in the compiler), but we might reach CONCAT_CONST_CV
>> or CONCAT_TMPVAR_CONST or some other combination thereof for which
>> this case is set to handle.
>>
>
> exectly.
> anyway CONCAT_CONST_CONST may be removed at all, at least to reduce the
> code size.
>
> TL;DR - Never mind me. I just didn't think it all the way through.
>>
>> -Sara
>>
>> P.S. - An assert(Z_TYPE_P(op1) == IS_STRING); and assert(Z_TYPE_P(op2)
>> == IS_STRING); does seem reasonable though... Maybe even with a
>> comment referencing that we can make that assumption because the
>> compiler fixes up the types.
>>
>
> ZEND_ASSERT() would be better.
>
> Thanks. Dmitry.
>
>
