Expand CHECK_SAME to non-jsvals in jsapi-tests

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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..."
Attachment #541815 - Flags: review?(luke)

Comment 1

6 years ago
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.
Attachment #541815 - Flags: review?(luke) → review+
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Comment 3

6 years ago
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.
Assignee: general → sphink
Attachment #541815 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #542907 - Flags: review?(luke)

Comment 4

6 years ago
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?
Attachment #542907 - Flags: review?(luke) → review+
(Assignee)

Comment 5

6 years ago
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.)
Created attachment 543352 [details] [diff] [review]
bustage fix for Win64

need long long define because Win64 is sizeof(void*) != sizeof(long).
Attachment #543352 - Flags: review?(luke)

Comment 7

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

Review of attachment 543352 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543352 - Flags: review?(luke) → review+
landed to fix win64 bustage
http://hg.mozilla.org/tracemonkey/rev/7eebf84c537f
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.
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-tracemonkey]
You need to log in before you can comment on or make changes to this bug.