code review 851047: Always treat the value column in kSuccessTable as a 64b...

Always treat the value column in kSuccessTable as a 64bit integer. Treating
it as other sizes of intiger only works on little-endian machines.

Fixes issue 11.

[Stefano Rivera, 2010-04-16 08:42:59]

Hello rsc (cc: re2-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-04-16 19:28:07]

http://codereview.appspot.com/851047/diff/2001/3001
File re2/testing/re2_arg_test.cc (right):

http://codereview.appspot.com/851047/diff/2001/3001#newcode17
re2/testing/re2_arg_test.cc:17: int16 value_int16;
Thanks for diagnosing this problem.
That's quite an embarrassing bug.

Instead of having all these fields, would it instead work
to have a single int64 value_int64, drop the ROW macro,
drop the braces around e.g. {127}, and then in the 
check below just cast the int64 value?

      ASSERT_EQUALS(r, (type)kSuccessTable[i].value_int64);

I'd check this but I don't have a big-endian machine to
play with.

Thanks.

[Stefano Rivera, 2010-04-19 14:31:15]

Hello rsc (cc: re2-dev@googlegroups.com),

Please take another look.

[Stefano Rivera, 2010-04-19 14:31:44]

http://codereview.appspot.com/851047/diff/2001/3001
File re2/testing/re2_arg_test.cc (right):

http://codereview.appspot.com/851047/diff/2001/3001#newcode17
re2/testing/re2_arg_test.cc:17: int16 value_int16;
On 2010/04/16 19:28:07, rsc1 wrote:
> Instead of having all these fields, would it instead work
> to have a single int64 value_int64, drop the ROW macro,
> drop the braces around e.g. {127}, and then in the 
> check below just cast the int64 value?

The uint64 test would then fail.

But yes, one can safely get rid of the 16 and 32 bit values. I'll submit an
update that does that now.

[rsc, 2010-04-19 14:39:37]

> The uint64 test would then fail.

Really?  I don't think it would.
Casting an int64 to a uint64 gives the right answer.

Russ

[Stefano Rivera, 2010-04-19 15:29:51]

On 2010/04/19 14:39:37, rsc wrote:
> Really?  I don't think it would.
> Casting an int64 to a uint64 gives the right answer.

Oh right, they are hex literals, treated as unsigned by the compiler.

[Stefano Rivera, 2010-04-19 15:30:55]

Hello rsc (cc: re2-dev@googlegroups.com),

Please take another look.

[rsc_swtch, 2010-04-23 00:47:52]

LGTM

thanks.

[rsc, 2010-04-23 00:49:03]

*** Submitted as http://code.google.com/p/re2/source/detail?r=31fb7640975c ***

re2_arg_test: use a single int64 instead of a union in kSuccessTable.
Union only works on little-endian machines.

Fixes issue 11.

R=rsc
CC=re2-dev
http://codereview.appspot.com/851047

Committer: Russ Cox <rsc@swtch.com>