threads / patch / 735

patchFix ptrdiff_t vs. int

Subject: [PATCH] Fix ptrdiff_t vs. int

## tl;dr

13 messages between May 27, 2005 and May 29, 2005. Diffs are folded; open one to read it.

replies: 12people: 6as markdown or json

Markus F.X.J. Oberhumer· May 27, 2005, 13:20 UTC · lore

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/







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++;
 			}
 		}
Danjel McGougan· May 27, 2005, 13:51 UTC · re: Markus F.X.J. Oberhumer · lore

Re: [PATCH] Fix ptrdiff_t vs. int

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
Morten Welinder· May 27, 2005, 14:02 UTC · re: Markus F.X.J. Oberhumer · lore

Re: [PATCH] Fix ptrdiff_t vs. int

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
H. Peter Anvin· May 27, 2005, 16:25 UTC · re: Markus F.X.J. Oberhumer · lore

Re: [PATCH] Fix ptrdiff_t vs. int

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
Linus Torvalds· May 27, 2005, 17:18 UTC · re: Markus F.X.J. Oberhumer · lore

Re: [PATCH] Fix ptrdiff_t vs. int

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
Linus Torvalds· May 27, 2005, 17:25 UTC · re: Linus Torvalds · lore

Re: [PATCH] Fix ptrdiff_t vs. int

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".
Show changes to diff-tree.c +5 −12
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);
Markus F.X.J. Oberhumer· May 27, 2005, 17:42 UTC · re: Linus Torvalds · lore

Re: [PATCH] Fix ptrdiff_t vs. int

Linus Torvalds wrote:
Show 10 quoted lines
> 
> 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
Show 47 quoted lines
> 
> 		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/
H. Peter Anvin· May 27, 2005, 17:58 UTC · re: Markus F.X.J. Oberhumer · lore

Re: [PATCH] Fix ptrdiff_t vs. int

Markus F.X.J. Oberhumer wrote:
Show 5 quoted lines
> 
> 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
Linus Torvalds· May 27, 2005, 18:13 UTC · re: Markus F.X.J. Oberhumer · lore

Re: [PATCH] Fix ptrdiff_t vs. int

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
Junio C Hamano· May 27, 2005, 18:20 UTC · re: Linus Torvalds · lore

Re: [PATCH] Fix ptrdiff_t vs. int

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

Junio C Hamano· May 28, 2005, 04:05 UTC · re: Junio C Hamano · lore

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

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(-)

Show changes to diff-helper.c +4 −1
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);
------------------------------------------------
Linus Torvalds· May 29, 2005, 18:31 UTC · re: Junio C Hamano · lore

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

On Fri, 27 May 2005, Junio C Hamano wrote:
Show 7 quoted lines
>
> 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
Junio C Hamano· May 29, 2005, 20:21 UTC · re: Linus Torvalds · lore

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

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

← back to recent threads