Avoid abs() of unsigned values.

Avoid abs() of unsigned values.


BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89173

[tzik, 2011-06-13 06:38:27]

Hi.

I found a abs() of size_t and comparision of possibly underflowed values, and
I'm trying to fix it.
Could you review this patch?

[Lei Zhang, 2011-06-13 22:01:15]

Hmm, the code here originally came from
http://code.google.com/p/google-gadgets-for-linux/source/browse/trunk/ggadget....
However, it doesn't even look right in some cases.

Let's rewrite the code based on the original spec:
http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#...

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc
File base/mime_util_xdg.cc (right):

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc#newcode328
base/mime_util_xdg.cc:328: return std::max(size, info->size) - std::min(size,
info->size);
can't we just do abs(size - info->size) instead?

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc#newcode330
base/mime_util_xdg.cc:330: if (size >= info->min_size && size <= info->max_size)
{
how about we rewrite this block to remove one layer of nested if/else:

if (size < info->min_size)
  return info->min_size - size;
else if (size > info->max_size)
  return size - info->max_size;
else
  return 0;

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc#newcode339
base/mime_util_xdg.cc:339: if (size + info->threshold >= info->size &&
Arg. I'm not even sure the old code was correct. :(

Again, let's rewrite this to be clearer:

if (size + info->threshold < info->size)
  return info->size - info->threshold - size;
else if (size > info->size + info->threshold)
  return size - info->size - info->threshold;
else
  return 0;

[tzik, 2011-06-14 02:52:52]

Thanks for reviewing.

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc
File base/mime_util_xdg.cc (right):

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc#newcode328
base/mime_util_xdg.cc:328: return std::max(size, info->size) - std::min(size,
info->size);
On 2011/06/13 22:01:15, Lei Zhang wrote:
> can't we just do abs(size - info->size) instead?

size - info->size is size_t expression, which is unsigned.
So, semantically abs(size - info->size) should equal to size - info->size.
Yet so stdlib.h in gcc provides abs(int), it is implicitly converted to int.

That makes warning with -Wconversion option in gcc, and makes error in other
implementation of library such as STLport.

So, I want to avoid using abs here.

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc#newcode330
base/mime_util_xdg.cc:330: if (size >= info->min_size && size <= info->max_size)
{
On 2011/06/13 22:01:15, Lei Zhang wrote:
> how about we rewrite this block to remove one layer of nested if/else:
> 
> if (size < info->min_size)
>   return info->min_size - size;
> else if (size > info->max_size)
>   return size - info->max_size;
> else
>   return 0;

Done.

http://codereview.chromium.org/7134090/diff/1/base/mime_util_xdg.cc#newcode339
base/mime_util_xdg.cc:339: if (size + info->threshold >= info->size &&
On 2011/06/13 22:01:15, Lei Zhang wrote:
> Arg. I'm not even sure the old code was correct. :(
> 
> Again, let's rewrite this to be clearer:
> 
> if (size + info->threshold < info->size)
>   return info->size - info->threshold - size;
> else if (size > info->size + info->threshold)
>   return size - info->size - info->threshold;
> else
>   return 0;

Done.

[Lei Zhang, 2011-06-14 03:02:27]

http://codereview.chromium.org/7134090/diff/4001/base/mime_util_xdg.cc
File base/mime_util_xdg.cc (right):

http://codereview.chromium.org/7134090/diff/4001/base/mime_util_xdg.cc#newcod...
base/mime_util_xdg.cc:342: return info->size + info->threshold - size;
I think this is reversed. You wrote, if (A > B) return B - A.

[tzik, 2011-06-14 03:40:25]

http://codereview.chromium.org/7134090/diff/4001/base/mime_util_xdg.cc
File base/mime_util_xdg.cc (right):

http://codereview.chromium.org/7134090/diff/4001/base/mime_util_xdg.cc#newcod...
base/mime_util_xdg.cc:342: return info->size + info->threshold - size;
On 2011/06/14 03:02:28, Lei Zhang wrote:
> I think this is reversed. You wrote, if (A > B) return B - A.

Done.

[Lei Zhang, 2011-06-14 20:23:02]

LGTM

[commit-bot: I haz the power, 2011-06-15 01:58:41]

Presubmit check for 7134090-10001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: base/mime_util_xdg.cc

Presubmit checks took 1.0s to calculate.

[Lei Zhang, 2011-06-15 02:22:08]

+evan or willchan for OWNER review.

[willchan no longer on Chromium, 2011-06-15 11:26:43]

LGTM rubberstamp

[commit-bot: I haz the power, 2011-06-15 13:14:31]

Change committed as 89173