Re: [PATCH v2 2/3] textconv: support for blame

From: Jeff King <peff@peff.net>
Date: Jun 15, 2010, 11:07 UTC
Message-ID: <20100615110710.GA1682@sigill.intra.peff.net>
In-Reply-To: <aad13a73928536f87879ef7284d6cc75@ensimag.fr>

[resending to cc git@vger]

On Tue, Jun 15, 2010 at 12:29:43PM +0200, bonneta wrote:

Show 10 quoted lines

> We have changed the declaration of textconv_object() to:
> 
> static int textconv_object(const char *path,
>                            const unsigned char *sha1,
>                            char **buf,
>                            unsigned long *buf_size)
> 
> And now we can do:
> *buf_size = fill_textconv(textconv, df, buf);
> without any cast.

I assume you mean dropping the final buf_size parameter from that declaration, which is what your usage example has. I would return either an "unsigned long" or a size_t rather than an int. We are dealing with potential whole-file sizes, so it is better to use at least as large a data type as other parts of the code (we still may run into truncation problems, but at least you are not making things any worse).

Show 6 quoted lines

> But we have to do:
> textconv_object(read_from, null_sha1, &buf.buf, (unsigned long *)
> &buf.len))
> where buf.len is size_t.
> 
> Is that ok?

No, that has the same problem. Imagine a big endian machine with a 32-bit unsigned long and a 64-bit size_t. You would write into the first 32 bits of buf.len, which are the high bits, giving you a ridiculously large answer.

The only portable way in C to convert between types is by assignment. So you have to do:

  unsigned long foo;
  textconv_object(read_from, null_sha1, &buf.buf, &foo);
  buf.len = foo;

But now I'm confused. That matches the declaration you gave in the first part of your email, but not the usage example.

-Peff