On Fri, Oct 30, 2009 at 12:12:40PM -0700, Shawn O. Pearce wrote:
> > I am just a bystander, so maybe my opinion is not worth anything, but
> > personally I think you are obfuscating the code to save a single line.
>
> Yup, me too. But I'm also willing to do what I need to get my
> patches included in git.git. Smart HTTP is something a lot of people
> have been waiting for. If the maintainer wants the bikeshed to be
> a particular shade of red, I'll paint it that way.
Yes, I too want it merged ASAP, so please Junio if you disagree with my
comments, ignore them. But I did want to point out that the proposed
"improvement" was making things worse, IMHO. So I feel like I was at
least bikeshedding his bikeshed, instead of starting my own. ;)
> > If you really just want to initialize to zero, using
> >
> > char linelen[5] = { 0 };
>
> Bleh, I find that has hard to grok as what we have now. Perhaps my
> understanding of the relevant standards is incomplete, but I'd read
> that as linelen[0] = 0, but the other 4 positions are undefined
> and may be not be initialized.
Your understanding is incomplete. :) C99, 6.7.8, paragraph 19:
... all subobjects that are not initialized explicitly shall be
initialized implicitly the same as objects that have static storage
duration.
I don't recall offhand whether that was the case in C89 or not.
But that being said, it was an attempt to make the meaning clear. If
it's not to you (who I consider at least reasonably competent ;) ), then
it failed.
> > die("protocol error: bad line length character: %.4s", linelen);
>
> I actually considered this one, but again I wasn't clear what would
> happen in the standard C library when we fed a string that wasn't
> actually NUL terminated. Is the library permitted to call strlen()
> before formatting? If so strlen() could SIGSEGV if we are unlucky
> and no NUL is present between our string and the end of the assigned
> memory region.
No, using a precision specifier for a string with no NUL is explicitly
allowed by the standard (and we use it elsewhere in git, IIRC).
> To me, my original version was the most clear, to me and anyone
> else who could ever possibly come by to read it. The "one extra
> line of code" is also only in an error condition which never occurs
> (but did once due to a bug in the HTTP code, which is why I added
> this patch to my series, to help debug it). Its not like this is
> a performance sensitive section of git that Linus is going to come
> back and overhaul.
Heh. You never know what performance item Linus will complain about. ;)
But yes, I am fine with your initial one, too. The drawback to it (and
my "%.4s") is not that one line of code is so unbearable, but that it is
one extra thing anyone using it as a string must do, and if they forget,
we get a horrible segfault.
Anyway, my complaint has been lodged. Junio is aware of the alternatives
and can pick whichever he wants.
-Peff
--
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