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.
