If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla33



JavaScript Engine
3 years ago
3 years ago


(Reporter: Waldo, Assigned: Waldo)



Firefox Tracking Flags

(Not tracked)



(2 attachments)

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.
Depends on: 1030515
Created attachment 8446278 [details] [diff] [review]
Attachment #8446278 - Flags: review?(jorendorff)
Comment on attachment 8446278 [details] [diff] [review]

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.
Attachment #8446278 - Flags: review?(jorendorff) → review+
Comment on attachment 8446278 [details] [diff] [review]

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...
Attachment #8446278 - Flags: review+
Created attachment 8446881 [details] [diff] [review]

In case you're interested. It's like 27 lines of new code, so, lame.
Comment on attachment 8446278 [details] [diff] [review]

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

Waldo pointed out that the patch in bug 1030515 copes with that. r+ restored.
Attachment #8446278 - Flags: review+
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.

Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.