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

On Tue, 15 Jun 2010 05:54:53 -0400, Jeff King [off-list ref] wrote:

On Tue, Jun 15, 2010 at 11:29:57AM +0200, Clément Poulain wrote:

quoted

quoted

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.

quoted

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...