Re: [PATCH] Location header recovering to absoluteURI

Hold on,

I agree completely with you in terms of performance, but the if statement is
not correct the way it stands. Shouldn't it be?


if (!((!trans->ssl && trans-port == 80) || (trans-ssl && trans-port ==
443)))
  octstr_format_append(os, ":%ld", trans->port);


BR,

Nikos

On Sun, Jan 25, 2009 at 6:44 PM, Stipe Tolj <[email protected]> wrote:

> Alexander Malysh schrieb:
> > Hi,
> >
> > some comments to patch:
> >
> > +        /* port, only added if literally not default. */
> > +        if (trans->port != 80) {
> > +            octstr_format_append(os, ":%ld", trans->port);
> > +        }
> >
> > I don't think this is needed, just always add port to URI.
>
> yep, but I have speed in mind here. The octstr_format_append() is
> "expensiver"
> in terms of CPU cycles then a simple long compare in the if statement. And
> we
> also don't need to "parse" it again in the next integration. That's why.
>
> > Otherwise patch look ok, +1 from me.
>
> ok, going to commit.
>
> Thanks Alejandro and Alex for reviewing.
> Stipe
>
> --
> -------------------------------------------------------------------
> Kölner Landstrasse 419
> 40589 Düsseldorf, NRW, Germany
>
> tolj.org system architecture      Kannel Software Foundation (KSF)
> http://www.tolj.org/              http://www.kannel.org/
>
> mailto:st_{at}_tolj.org <st_%7Bat%7D_tolj.org>           mailto:
> stolj_{at}_kannel.org <stolj_%7Bat%7D_kannel.org>
> -------------------------------------------------------------------
>
>