Re: [PATCH v2 2/3] textconv: support for blame
- From
- bonneta <bonneta@ensimag.fr>
- Date
- Jun 15, 2010, 10:32 UTC
- Message-ID
- <192517e06785fed4fa799bee9a11ae28@ensimag.fr>
- In-Reply-To
- <20100615095452.GA32624@sigill.intra.peff.net>
On Tue, 15 Jun 2010 05:54:53 -0400, Jeff King <peff@peff.net> wrote:
Show 33 quoted lines
> On Tue, Jun 15, 2010 at 11:29:57AM +0200, Clément Poulain wrote:
>
>> > The same issue exists in Clément's patch to builtin/cat-file.c.
>>
>> We did this way because we found a similar cast in prep_temp_blob(),
>> diff.c:
>>
>> if (convert_to_working_tree(path,
>> (const char *)blob, (size_t)size, &buf)) {
>>
>> where size is an unsigned long.
>> Is it the same issue ? Or is it different because it's not a pointer
>> cast?
>
> Right. The compiler will handle conversion between integer types during
> assignment itself, converting representations as necessary (in fact,
> that cast looks useless to me, as implicit conversions are allowed in
> C). The only problem is dereferencing a pointer to X as something other
> than X.
>
>> Otherwise, we thought of reversing the conversion. That is to say,
>> instead
>> of casting "long *" in "size_t *" when calling textconv_object(), is it
>> better to cast size_t in "unsigned long" in textconv_object():
>>
>> *buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?
>
> You shouldn't even have to cast there, for the same reason as above.
> That is why I wrote fill_textconv to return the size parameter, rather
> than writing to a passed-in pointer. It avoids the annoying
> size_t / unsigned long casting caused by different usage (in an ideal
> world, all of our sizes would be the same type, but the strbuf and diff
> code obviously differ).Thanks for your answer.
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.
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? Our gcc doesn't report any strict-aliasing problem, so we don't know if it is better than the initial version or not...