On Tue, 21 Jan 2014, Daniel Stenberg wrote:

> > Kindly review the code changes and provide your valuable feedback.
>
> Awesome contributition Prash. Let's get this in shape for merging
> immediately after the next release!

The patch looks pretty straight forward to me.

> 1. Okay, first I think you should rebase your work on top of the current
> git HEAD. Then you'll see that we don't have curl_rand.h anymore!

Whilst rebasing it, and as you'll be using git, are you able to provide a
patch generated using git format please?

That way I can quickly apply it over my local repo and test it against an
instance of Exchange Server - assuming that it supports NTLMv2 which I would
hope it does;-)

> 3. You also need to pay slightly closer attention to the code style you
> use, which you can check with this:
>
> "make checksrc"
>
> It shows 93 warnings right now (most of them being for the same same
> error on multiple places), like TABS or trailing whitespaces. Also note
how we
> position the open braces for if/while/do expressions.

In addition to Daniels comments and the errors that checksrc will generate,
would you be so kind to:

* Make the most of the 79 character limit we have - I noticed a few examples
where you truncate a line at 60 characters for example in both code and
comments / function descriptions
* Rather than explicitly checking against CURLE_OUT_OF_MEMORY for example, I
would recommend returning the result into a "result" type variable and
returning that. This means that if function returns multiple error results
they will all be caught and if it only returns CURLE_OUT_OF_MEMORY it future
proofs it to a certain degree ;-)
* For consistency, and I see you have used this style in place, try and use
! on "if statement" comparisons when checking result codes or for NULL
pointers rather than an explicit comparison
* No need for a space between an ! and the comparison value
* When function argument definitions run over the 79 character limit and run
onto the second line, please place the next argument in line with the first,
again, like you have done in some places
* If an "if statement" only does one thing, there is no need for the curly
braces above and below it
* I would recommend using Curl_safefree() rather than free() due to the way
it integrates with our tests and valgrind - It also means that you don't
have to do value = NULL afterwards as the macro does it for you
* int64_t might not be supported on all platforms - would possibly recommend
using long long or size_t (as we defined that ourselves where it isnt't
supported) - the latter if the variable doesn't need to be signed

I hope I haven't been too harsh there and many thanks for your efforts so
far

Kind Regards

Steve
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-01-21