Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King <peff@peff.net>]

On Wed, Sep 16, 2015 at 12:07:30PM -0700, Junio C Hamano wrote:

> > Correct. The original is sane and gcc does the right thing. The question
> > is whether some compiler would complain that "stage" is not a constant
> > in the sizeof() expression. I don't know if any compiler would do so,
> > but it is easy enough to be conservative.
> 
> Wouldn't such a compiler also complain if you did this, though?
> 
> 	int *pointer_to_int;
>         size_t sz = sizeof(*pointer_to_int);
>
> You (as a complier) do not know exactly where ownbuf[stage] is,
> because "stage" is unknown to you.  In the same way, you do not know
> exactly where *pointer_to_int is.  But of course, what the sizeof()
> operator is being asked is the size of the thing, which does not
> depend on where the thing is.  If you (as a compiler) does not know
> that and complain to ownbuf[stage], wouldn't you complain to the
> pointer dereference, too?

I think it depends on how crappily the compiler is implemented. I agree
that sizeof(ownbuf[stage]) is a reasonable thing to ask for without
knowing the exact value of stage. But I could also see it being a
mistake an old or badly written compiler might make.

But we are theorizing without data. I'm happy to leave it as in my
original and wait to see if anybody ever complains.

> A more important reason I am reluctant to see this:
> 
> 	xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
> 
> is that it looks strange in the same way as this
> 
> 	memcpy(ownbuf[stage], src, sizeof(ownbuf[0]));
> 
> looks strange.  "We use the size of one thing to stuff into another".
> 
> That will make future readers wonder "Is this a typo, and if it is,
> which index is a mistake I can fix?" and may lead to an unnecessary
> confusion.  I do not want to see a correctly written
> 
> 	xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
> 
> turned into
> 
> 	xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", ...);
> 
> out of such a confusion.

I think that's a reasonable concern.

-Peff