667076 - Expand CHECK_SAME to non-jsvals in jsapi-tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Attachment: Make CHECK_SAME handle more types

CHECK failed at line 323.

"Oh, man, I have to add these same printfs again??!!"
"Isn't here something in the test infrastructure like IS(a,b) that'll report the old and new values, so I don't have to print them out myself?"
"Oh, wait, there is! CHECK_SAME! Coolness!"
"Oh. It's only for jsvals. I have plain ints."
"Hey, I could just make it templatize on the type. It'll only take a couple of minutes..."
"Oh, I have to display the values, too. More templates..."
"...and explicit instantations..."
"Arg! It's common to have two different types, like jsvalRoot and jsval. More template-y bizarreness..."

CHECK_SAME failed at line 323: expected 1+1 = 2, got enters = 1.

"Phew. What a mess. Is this even a good idea any more? Maybe I should've added CHECK_SAME_INT and been done with it. The output is a little hard to follow, too."
"I know! I'll flag luke for review and make him decide! Bwahahaahaha!!"
"Y'know, if I'd just fixed the bug instead of messing around with the test infrastructure, I would've been done hours ago..."

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 541815 [details] [diff] [review]
Make CHECK_SAME handle more types

Looks useful, nice.  In situations like toSource, there isn't much of an advantage to using templates+specialization so generally I'd prefer to use plain old overloading.  I guess it saves you the declarations, but, with overloading, you could define the overloads inline.  Your choice.

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to comment #1)
> Comment on attachment 541815 [details] [diff] [review] [review]
> Make CHECK_SAME handle more types
> 
> Looks useful, nice.  In situations like toSource, there isn't much of an
> advantage to using templates+specialization so generally I'd prefer to use
> plain old overloading.  I guess it saves you the declarations, but, with
> overloading, you could define the overloads inline.  Your choice.

That's a very nice way of putting it. Another way, which I'd use if I were talking to myself, would be "Umm... what idiot uses fancy template-fu when plain overloading would do?!"

See? The rhyme takes away some of the sting.

I switched to overloading, thanks. I was just stuck in the template mindset.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Add a CHECK_EQUAL for non-jsval types

Sorry, I should have pushed to try before requesting review.

The patch fails on opt builds, because jsval interferes with whatever integer type it is equivalent to. It worked for me because I have a debug build where jsval is a struct.

I could do creative ifdeffing to exclude whichever integer type matches jsval, but then I could erroneously try to convert a regular number to a jsval. So I retreated and made a new CHECK_EQUAL for non-jsval types, and left CHECK_SAME for jsvals.

Naming suggestions welcome.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 542907 [details] [diff] [review]
Add a CHECK_EQUAL for non-jsval types

Review of attachment 542907 [details] [diff] [review]:
-----------------------------------------------------------------

I actually needed this yesterday so glad to see it land.

::: js/src/jsapi-tests/testFuncCallback.cpp
@@ +65,5 @@
> +    CHECK_EQUAL(leaves, 1);
> +    CHECK_EQUAL(depth, 0);
> +    CHECK_EQUAL(enters, 1);
> +    CHECK_EQUAL(leaves, 1);
> +    CHECK_EQUAL(depth, 0);

x2?

Comment 5

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/60413613ea2a

(In reply to comment #4)
> ::: js/src/jsapi-tests/testFuncCallback.cpp
> @@ +65,5 @@
> > +    CHECK_EQUAL(leaves, 1);
> > +    CHECK_EQUAL(depth, 0);
> > +    CHECK_EQUAL(enters, 1);
> > +    CHECK_EQUAL(leaves, 1);
> > +    CHECK_EQUAL(depth, 0);
> 
> x2?

In school I learned to practice bullet-proof coding. What kind of cut-rate school did you go to? What if a cosmic ray flipped a bit between checks? Cosmic rays are bullets too!

Fixed.

Oh, and the version I committed removes some "dead" code. (It wasn't really dead, but it should be; it would do the wrong thing if you used CHECK_EQUAL to compare the integer type matching jsval.)

Comment 6

[Makoto Kato [:m_kato]]

Attachment: bustage fix for Win64

need long long define because Win64 is sizeof(void*) != sizeof(long).

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 543352 [details] [diff] [review]
bustage fix for Win64

Review of attachment 543352 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 8

[Makoto Kato [:m_kato]]

landed to fix win64 bustage
http://hg.mozilla.org/tracemonkey/rev/7eebf84c537f

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/60413613ea2a
http://hg.mozilla.org/mozilla-central/rev/7eebf84c537f
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.