1030521 - Lots of signed/unsigned comparison warnings in tests.h checkEqual instantiations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

This wasn't an issue before because checkEqual took actual/expected arguments of the same type, so the comparison was always sane.  Now, if we're comparing different types, this issue can manifest.  Make it into a compile error so people can't screw it up and clog up build output.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8446278 [details] [diff] [review]
Patch

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

Looks OK, r=me.

I think it would be better to disable the warning for that line in tests.h. The reason I think it's OK to silence the warning here is that it's an assertion with an 'actual' and 'expected' value, and it's very likely that the value of 'expected' is a constant, most likely a literal, and small and nonnegative. In other words, I claim the warning is not likely to point out any actual bugs in the future lifetime of the project.

A third possibility is the patch I will attach in a second, kind of a counter-offer. I don't feel strongly about it -- any of the 3 fixes is fine with me.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8446278 [details] [diff] [review]
Patch

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

Wait a minute, this doesn't build for me in clang. I get errors like this:

In file included from /home/jorendorff/dev/mozilla-inbound/js/src/clang-obj/js/src/jsapi-tests/Unified_cpp_js_src_jsapi-tests3.cpp:2:
In file included from /home/jorendorff/dev/mozilla-inbound/js/src/jsapi-tests/testOriginPrincipals.cpp:7:
/home/jorendorff/dev/mozilla-inbound/js/src/jsapi-tests/../jsapi-tests/tests.h:168:43: error: 
      incomplete definition of type 'mozilla::IsSigned<JS::Rooted<JSAtom *> >'
        static_assert(mozilla::IsSigned<T>::value == mozilla::IsSigned<U>::value,
                      ~~~~~~~~~~~~~~~~~~~~^~
/home/jorendorff/dev/mozilla-inbound/js/src/jsapi-tests/testStringBuffer.cpp:26:5: note: in
      instantiation of function template specialization 'JSAPITest::checkEqual<JS::Rooted<JSAtom *>,
      JS::Rooted<JSAtom *> >' requested here
    CHECK_EQUAL(atom, finishedAtom);
    ^
/home/jorendorff/dev/mozilla-inbound/js/src/jsapi-tests/../jsapi-tests/tests.h:180:14: note: 
      expanded from macro 'CHECK_EQUAL'
        if (!checkEqual(actual, expected, #actual, #expected, __FILE__, __LINE__)) \
             ^

Clearing review for the moment...

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: bug-1030521-counteroffer.patch

In case you're interested. It's like 27 lines of new code, so, lame.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8446278 [details] [diff] [review]
Patch

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

Waldo pointed out that the patch in bug 1030515 copes with that. r+ restored.

Comment 6

[Jeff Walden [:Waldo]]

That patch wouldn't be enough, I think.  Only checks whose argument types *exactly* match the overload argument types, would select those new overloads.  You'd need a mess of SFINAE EnableIf sorts of things to ensure signed-unsigned comparisons selected only an overload that worked that way.  And those would have to coordinate among themselves, in effect, to ensure they all covered non-overlapping areas.  It'd be really messy.

The patch here might require a few 'u' suffixes, but in the end that seems less painful than, as noted on IRC, requiring detailed knowledge of the interaction between overload selection, template selection, best viable matching, and all that stuff.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c4cde5836b3

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3c4cde5836b3