Closed
Bug 667076
Opened 13 years ago
Closed 13 years ago
Expand CHECK_SAME to non-jsvals in jsapi-tests
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sfink, Assigned: sfink)
Details
(Whiteboard: [fixed-in-tracemonkey])
Attachments
(2 files, 1 obsolete file)
24.22 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
975 bytes,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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•13 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•13 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•13 years ago
|
||
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•13 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•13 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.)
Comment 6•13 years ago
|
||
need long long define because Win64 is sizeof(void*) != sizeof(long).
Attachment #543352 -
Flags: review?(luke)
Comment 7•13 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+
Comment 8•13 years ago
|
||
landed to fix win64 bustage http://hg.mozilla.org/tracemonkey/rev/7eebf84c537f
Comment 9•13 years ago
|
||
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•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-tracemonkey]
You need to log in
before you can comment on or make changes to this bug.
Description
•