Re: [PATCH v2] blame: add a range option to -L

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 4, 2010, 18:11 UTC
Message-ID: <vpqk4rj8rks.fsf@bauges.imag.fr>
In-Reply-To: <1272909995-3198-1-git-send-email-wfp5p@virginia.edu>

Bill Pemberton <wfp5p@virginia.edu> writes:

>  		term = parse_loc(term + 1, sb, lno, *bottom + 1, top);
> -		if (*term)
> -			usage(blame_usage);
> +		x = *top;

Why not use parse_loc(..., &x) if you want the value to end up in x ?

> +		*top = *bottom - x;
> +		*bottom += x;

The existing code seems to assume that top >= bottom, but swaps top and bottom otherwise:

	if (bottom && top && top < bottom) {
		long tmp;
		tmp = top; top = bottom; bottom = tmp;
	}

So, I'd write

*top = *bottom + x; *bottom -= x;

> +		if (*bottom < 1)
> +			*bottom = 1;

I guess you've assumed that bottom was the small number here, otherwise, you're checking for overflow, not for actually negative numbers. Either you apply my proposal above or you should s/bottom/top/ here, right?

(the existing code already have this a few lines after the call to this functions, it doesn't harm to do it again, but better do it on the right function)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/