Hi,

Steve Holme wrote:

> I wanted to churp up and provide my feedback as

Thanks for the quick review.

[...]
> * You seemed to have changed the use of Curl_safefree() to free() in the
> sasl rework, when variables such as chlg and response could be NULL, but use
> a mixture of free() and Curl_safefree() elsewhere. There are some cases when
> the pointer is already being checked and as such free() could be more
> appropriate but as Curl_safefree() also invalidates the pointer by setting
> it to NULL I would recommend using this over free() - It then means you
> don't need to set variables like "proxy" to NULL afterwards as
> Curl_safefree() will do it for you.

Yes, the series isn't very consistent about this. The change to
"proxy" handling I mentioned was just a free() -> Curl_safefree()
change, by the way. I agree that in code like this which isn't too
performance-critical, it's easiest on the reader to always use
Curl_safefree(), so I'll do that.

> * Why change the existing "if(failure), set return code, free and return" to
> "if(failure), set return code, goto out and then free" ?

It makes it easier to remember to free everything.

> Additionally, if there is a valid
> reason for changing the coding style could you please perform this as part
> of a post fix tidy up in separate patches - thus keeping the fix and style
> changes separate, which will allow easier reviewing and allow for the
> dissecting of problems should there be any.

I like to do this in the opposite order: first style cleanup that will
make the later patches easier to read, then minimal patches that make
the appropriate functional changes on top. That way, the style fix
and the new behavior can be reviewed separately and there is an
example to make the impact of each style fix more concrete.

In the case of e.g. a security or recent regression fix that needs to
be backported, though, I do it in the order you say: first the fix;
then, after the dust has settled, style fixes. Hopefully these
patches don't fall in that category.

Did any of the patches in particular stand out as potentially needing
to be split?

> Additionally, is there any possibility you could attach your .patch files
> rather than sending them as separate emails?

I'm happy to follow whatever the local convention is. I have a mild
preference of one patch per email because it allows them to be
discussed separately.

Will reroll with fewer uses of "free()" and with the asprintf change
Daniel suggested.

Ciao,
Jonathan
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-08-20