Last Comment Bug 667076 - Expand CHECK_SAME to non-jsvals in jsapi-tests
: Expand CHECK_SAME to non-jsvals in jsapi-tests
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Steve Fink [:sfink] [:s:] (PTO Sep23-28)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-24 15:01 PDT by Steve Fink [:sfink] [:s:] (PTO Sep23-28)
Modified: 2011-07-06 10:10 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make CHECK_SAME handle more types (24.44 KB, patch)
2011-06-24 15:01 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
luke: review+
Details | Diff | Splinter Review
Add a CHECK_EQUAL for non-jsval types (24.22 KB, patch)
2011-06-29 12:25 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
luke: review+
Details | Diff | Splinter Review
bustage fix for Win64 (975 bytes, patch)
2011-06-30 22:34 PDT, Makoto Kato [:m_kato]
luke: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2011-06-24 15:01:00 PDT
Created attachment 541815 [details] [diff] [review]
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] 2011-06-24 15:32:24 PDT
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:] (PTO Sep23-28) 2011-06-24 17:53:27 PDT
(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:] (PTO Sep23-28) 2011-06-29 12:25:06 PDT
Created attachment 542907 [details] [diff] [review]
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] 2011-06-29 18:06:15 PDT
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:] (PTO Sep23-28) 2011-06-30 13:16:17 PDT
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] 2011-06-30 22:34:45 PDT
Created attachment 543352 [details] [diff] [review]
bustage fix for Win64

need long long define because Win64 is sizeof(void*) != sizeof(long).
Comment 7 Luke Wagner [:luke] 2011-06-30 22:47:06 PDT
Comment on attachment 543352 [details] [diff] [review]
bustage fix for Win64

Review of attachment 543352 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 8 Makoto Kato [:m_kato] 2011-07-01 00:03:47 PDT
landed to fix win64 bustage
http://hg.mozilla.org/tracemonkey/rev/7eebf84c537f
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-07-05 18:06:19 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.