Re: [PATCH] implemented strbuf_write_or_die()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Please leave a little more time for people to give feedback between
versions of a patch series (unless the first version was so broken that
it would be pointless for any other reviewer to waste time on it.  And
please label the versions of a single patch series "[PATCH]" then
"[PATCH v2]", "[PATCH v3]", etc.

I agree with Johannes's advice that the function is in the wrong place
and has the wrong parameter order.

On 03/01/2014 02:29 PM, Faiz Kothari wrote:
> Signed-off-by: Faiz Kothari <faiz.off93@xxxxxxxxx>
> ---
>>> -               write_or_die(1, rpc.result.buf, rpc.result.len);
>>> +               strbuf_write_or_die(1, &(rpc.result.buf));
> 
>> May be this should be
>> strbuf_write_or_die(1, &(rpc.result));
> 
> Yes, I changed that :-) Thanks again.

I find it alarming that either the compiler didn't emit warnings for the
old version or that you ignored the compiler warnings.  Git should
compile without warnings even with with quite strict compiler settings;
I use gcc with the following options

    -Wall -Werror \
        -Wdeclaration-after-statement \
        -Wno-format-zero-length \
        -Wno-format-security

Maybe you weren't including the header file that declares
strbuf_write_or_die() in the file containing this invocation?

Also, the parentheses in "&(rpc.result)" are unnecessary.

And I think that some of the blank lines that you added contained
invisible whitespace.  Please check your whitespace!  You can run "git
diff --check" to detect some obvious whitespace problems.

Michael

-- 
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]