[PATCH] Fix ptrdiff_t vs. int

[PATCH] Fix ptrdiff_t vs. int

[Markus F.X.J. Oberhumer]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

This trivial patch fixes an obvious ptrdiff_t vs. int mismatch. Which
makes we wonder why Linus isn't hitting this on his ppc64 - maybe it's
time to start using -Werror...

Signed-off-by: Markus F.X.J. Oberhumer <markus@oberhumer.com>

-- 
Markus Oberhumer, <markus@oberhumer.com>, http://www.oberhumer.com/






[-- Attachment #2: diff-tree.c.patch --]
[-- Type: text/x-patch, Size: 598 bytes --]

This trivial patch fixes an obvious ptrdiff_t vs. int mismatch.

Signed-off-by: Markus F.X.J. Oberhumer <markus@oberhumer.com>

Index: diff-tree.c
===================================================================
--- 1348af9952a1d26b2ad14ec8f433322fd79510f3/diff-tree.c  (mode:100644)
+++ 61dcf68d605a8d9204c24278dbdc73b4cf7ccc90/diff-tree.c  (mode:100644)
@@ -274,7 +274,7 @@
 			for (cp = header; *cp; cp = ep) {
 				ep = strchr(cp, '\n');
 				if (ep == 0) ep = cp + strlen(cp);
-				printf("%.*s%c", ep-cp, cp, 0);
+				printf("%.*s%c", (int) (ep-cp), cp, 0);
 				if (*ep) ep++;
 			}
 		}

Re: [PATCH] Fix ptrdiff_t vs. int

[Danjel McGougan]

Markus F.X.J. Oberhumer wrote:
> This trivial patch fixes an obvious ptrdiff_t vs. int mismatch. Which
> makes we wonder why Linus isn't hitting this on his ppc64 - maybe it's
> time to start using -Werror...
> 

I think the PPC calling convention is to use registers for the first few 
parameters, so the bug probably will not surface on PPC64.

/Danjel

Re: [PATCH] Fix ptrdiff_t vs. int

[Morten Welinder]

On 5/27/05, Markus F.X.J. Oberhumer <markus@oberhumer.com> wrote:
> This trivial patch fixes an obvious ptrdiff_t vs. int mismatch. Which
> makes we wonder why Linus isn't hitting this on his ppc64 - maybe it's
> time to start using -Werror...

The best time to start using -Werror is "never".  Different compilers
(and versions)
warn about different things, often affected by, say, optimization
switches.  Different
system headers contain things that one compiler or another will issue a warning
over.

Thus -Werror is solely for code that needs to work on one compiler, with one set
of switches, with one libc version, and specific versions of included libraries.

It is tempting to submit a patch to the gcc people making -Werror issue a
warning about the trouble using -Werror.

Morten

Re: [PATCH] Fix ptrdiff_t vs. int

[H. Peter Anvin]

Markus F.X.J. Oberhumer wrote:
> This trivial patch fixes an obvious ptrdiff_t vs. int mismatch. Which
> makes we wonder why Linus isn't hitting this on his ppc64 - maybe it's
> time to start using -Werror...

This doesn't affect CPUs with a register-based calling convention.

	-hpa

Re: [PATCH] Fix ptrdiff_t vs. int

[Linus Torvalds]

On Fri, 27 May 2005, Markus F.X.J. Oberhumer wrote:
>
> This trivial patch fixes an obvious ptrdiff_t vs. int mismatch. Which
> makes we wonder why Linus isn't hitting this on his ppc64 - maybe it's
> time to start using -Werror...

My _kernel_ is 64-bit, but my user-space is all 32-bit because (a) I don't
care about user-space and (b) I'm lazy and (c)  all the distributions
contain just 32-bit programs and a ppc64 kernel has no trouble running
32-bit programs.

I can compile a kernel with "-m64", but since I don't have any 64-bit
libraries installed, user space doesn't work that well ;)

		Linus

Re: [PATCH] Fix ptrdiff_t vs. int

[Linus Torvalds]

On Fri, 27 May 2005, Linus Torvalds wrote:
> 
> I can compile a kernel with "-m64", but since I don't have any 64-bit
> libraries installed, user space doesn't work that well ;)

Btw, since this was the piece of code that I didn't bother simplifying 
last time it was discussed (then it was just "ugly"), I took a different 
approach instead, and committed the following..

		Linus

---
diff-tree 84c1afd7e7c69c6c3c0677d5ee01925d4c70d318 (from a9c9cef161b26ca610783dd0b180d18956c7b119)
Author: Linus Torvalds <torvalds@ppc970.osdl.org>
Date:   Fri May 27 10:22:09 2005 -0700
    
    git-diff-tree: simplify header output with '-z'
    
    No need to make them multiple lines, in fact we explicitly don't want that.
    
    This also fixes a 64-bit problem pointed out by Markus F.X.J. Oberhumer,
    where we gave "%.*s" a "ptrdiff_t" length argument instead of an "int".

diff --git a/diff-tree.c b/diff-tree.c
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -269,18 +269,11 @@ static int call_diff_flush(void)
 		return 0;
 	}
 	if (header) {
-		if (diff_output_format == DIFF_FORMAT_MACHINE) {
-			const char *ep, *cp;
-			for (cp = header; *cp; cp = ep) {
-				ep = strchr(cp, '\n');
-				if (ep == 0) ep = cp + strlen(cp);
-				printf("%.*s%c", ep-cp, cp, 0);
-				if (*ep) ep++;
-			}
-		}
-		else {
-			printf("%s", header);
-		}
+		const char *fmt = "%s";
+		if (diff_output_format == DIFF_FORMAT_MACHINE)
+			fmt = "%s%c";
+		
+		printf(fmt, header, 0);
 		header = NULL;
 	}
 	diff_flush(diff_output_format, 1);

Re: [PATCH] Fix ptrdiff_t vs. int

[Markus F.X.J. Oberhumer]

Linus Torvalds wrote:
> 
> On Fri, 27 May 2005, Linus Torvalds wrote:
> 
>>I can compile a kernel with "-m64", but since I don't have any 64-bit
>>libraries installed, user space doesn't work that well ;)
> 
> 
> Btw, since this was the piece of code that I didn't bother simplifying 
> last time it was discussed (then it was just "ugly"), I took a different 
> approach instead, and committed the following..

Many thanks. BTW, this is probably completely irrelevant on all 
real-world machines and calling conventions, but still, lying on the 
number of arguments in vararg-functions is not my favourite practice.

~Markus

> 
> 		Linus
> 
> ---
> diff-tree 84c1afd7e7c69c6c3c0677d5ee01925d4c70d318 (from a9c9cef161b26ca610783dd0b180d18956c7b119)
> Author: Linus Torvalds <torvalds@ppc970.osdl.org>
> Date:   Fri May 27 10:22:09 2005 -0700
>     
>     git-diff-tree: simplify header output with '-z'
>     
>     No need to make them multiple lines, in fact we explicitly don't want that.
>     
>     This also fixes a 64-bit problem pointed out by Markus F.X.J. Oberhumer,
>     where we gave "%.*s" a "ptrdiff_t" length argument instead of an "int".
> 
> diff --git a/diff-tree.c b/diff-tree.c
> --- a/diff-tree.c
> +++ b/diff-tree.c
> @@ -269,18 +269,11 @@ static int call_diff_flush(void)
>  		return 0;
>  	}
>  	if (header) {
> -		if (diff_output_format == DIFF_FORMAT_MACHINE) {
> -			const char *ep, *cp;
> -			for (cp = header; *cp; cp = ep) {
> -				ep = strchr(cp, '\n');
> -				if (ep == 0) ep = cp + strlen(cp);
> -				printf("%.*s%c", ep-cp, cp, 0);
> -				if (*ep) ep++;
> -			}
> -		}
> -		else {
> -			printf("%s", header);
> -		}
> +		const char *fmt = "%s";
> +		if (diff_output_format == DIFF_FORMAT_MACHINE)
> +			fmt = "%s%c";
> +		
> +		printf(fmt, header, 0);
>  		header = NULL;
>  	}
>  	diff_flush(diff_output_format, 1);
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Markus Oberhumer, <markus@oberhumer.com>, http://www.oberhumer.com/

Re: [PATCH] Fix ptrdiff_t vs. int

[H. Peter Anvin]

Markus F.X.J. Oberhumer wrote:
> 
> Many thanks. BTW, this is probably completely irrelevant on all 
> real-world machines and calling conventions, but still, lying on the 
> number of arguments in vararg-functions is not my favourite practice.
> 

Bullshit.

Think about the way stdarg works, and you should quickly figure out that 
it is perfectly safe to pass extra arguments.

However, this code can also be rewritten:

	fputs(header, stdout);
	if (diff_output_format == DIFF_FORMAT_MACHINE)
		putchar('\0');

... which is probably faster anyway.

	-hpa

Re: [PATCH] Fix ptrdiff_t vs. int

[Linus Torvalds]

On Fri, 27 May 2005, Markus F.X.J. Oberhumer wrote:
> 
> Many thanks. BTW, this is probably completely irrelevant on all 
> real-world machines and calling conventions, but still, lying on the 
> number of arguments in vararg-functions is not my favourite practice.

Only using a subset is guaranteed to work - it's not about "real-world 
machines", it's about the fact that it's not a C compiler any more if it 
doesn't work.

		Linus

Re: [PATCH] Fix ptrdiff_t vs. int

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> On Fri, 27 May 2005, Linus Torvalds wrote:
>> 
>> I can compile a kernel with "-m64", but since I don't have any 64-bit
>> libraries installed, user space doesn't work that well ;)

LT> Btw, since this was the piece of code that I didn't bother simplifying 
LT> last time it was discussed (then it was just "ugly"), I took a different 
LT> approach instead, and committed the following..

I thought about this after getting your "Your solution is the
yucky one" message, but make sure that you are not adding an
extra blank line when this is combined with diff-helper.  I
think you currently do.

[PATCH] Adjust diff-helper to diff-tree -v -z changes.

[Junio C Hamano]

The latest change to diff-tree -z output adds an extra line
termination after non diff-raw material (the header and the
commit message).  To compensate for this change, stop adding the
output termination of our own.  "diff-tree -v -z" piped to
"diff-helper -z" would give different result from "diff-tree -v"
piped to "diff-helper" without this change.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

diff-helper.c |    5 ++++-
1 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/diff-helper.c b/diff-helper.c
--- a/diff-helper.c
+++ b/diff-helper.c
@@ -15,6 +15,7 @@ static const char *diff_helper_usage =
 
 int main(int ac, const char **av) {
 	struct strbuf sb;
+	const char *garbage_flush_format;
 
 	strbuf_init(&sb);
 
@@ -30,6 +31,8 @@ int main(int ac, const char **av) {
 			usage(diff_helper_usage);
 		ac--; av++;
 	}
+	garbage_flush_format = (line_termination == 0) ? "%s" : "%s\n";
+
 	/* the remaining parameters are paths patterns */
 
 	diff_setup(0);
@@ -134,7 +137,7 @@ int main(int ac, const char **av) {
 		if (pickaxe)
 			diffcore_pickaxe(pickaxe, pickaxe_opts);
 		diff_flush(DIFF_FORMAT_PATCH, 0);
-		printf("%s\n", sb.buf);
+		printf(garbage_flush_format, sb.buf);
 	}
 	if (1 < ac)
 		diffcore_pathspec(av + 1);
------------------------------------------------

Re: [PATCH] Adjust diff-helper to diff-tree -v -z changes.

[Linus Torvalds]

On Fri, 27 May 2005, Junio C Hamano wrote:
>
> The latest change to diff-tree -z output adds an extra line
> termination after non diff-raw material (the header and the
> commit message).  To compensate for this change, stop adding the
> output termination of our own.  "diff-tree -v -z" piped to
> "diff-helper -z" would give different result from "diff-tree -v"
> piped to "diff-helper" without this change.

I think this is really a bug in your "read_line()" interface.

You should include the terminating character in the line count.

This also fixes and simplifies "EOF" handling.

		Linus

Re: [PATCH] Adjust diff-helper to diff-tree -v -z changes.

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> I think this is really a bug in your "read_line()" interface.
LT> You should include the terminating character in the line count.

What the patch changed:

    You give me a non diff-raw material "foo\nbar\n" when you
    are not doing -z.  I read that without -z.  read_line()
    drops the EOL so I get "foo" and "bar" on separate lines,
    both of which I spit out with my own '\n' using "%s\n".

    You give me "foo\nbar\n\0" to express the same under -z, and
    I read that with -z.  I get "foo\nbar\n" after read_line()
    drops the EOL.  I should spit it out without my own '\n',
    i.e. not using "%s\n" but "%s".

If read_line() interface changes to include EOL, then...

    You give me "foo\nbar\n" without -z.  I read that without
    -z.  Fixed read_line() retains the EOL and I get "foo\n" and
    "bar\n", and I do not have to add my own '\n' anymore; I
    just do fputs().

    You give me "foo\nbar\n\0" under -z, and I read that with
    -z.  Fixed read_line() retains the EOL so I get
    "foo\nbar\n\0".  I just do fputs() and it would drop the
    '\0'.

What the last sentence does feels a bit hacky, but does the
right thing.  It's a good fix.

Please discard the patch you are responding to unless you
already have applied it.