Botond Ballo [:botond] Assignee
	Description • 10 years ago

STR:
  1. Write a strongly-typed enumeration with an unsigned underlying type.
  2. Write a ParamTraits specialization for it that uses TypedEnumSerializer.
  3. Try to compile the code.

Expected:
  Code compiles fine.

Actual:
  ipc/IPCMessageUtils.h: In instantiation of 'static bool IPC::TypedEnumSerializer<E, smallestLegal, highBound>::IsLegalValue(const paramType&) [with E = mozilla::layers::GeckoContentController::APZStateChange; E smallestLegal = (mozilla::layers::GeckoContentController::APZStateChange)0u; E highBound = (mozilla::layers::GeckoContentController::APZStateChange)5u; IPC::TypedEnumSerializer<E, smallestLegal, highBound>::paramType = mozilla::layers::GeckoContentController::APZStateChange]':
  ipc/IPCMessageUtils.h:166:5:   required from 'static bool IPC::TypedEnumSerializer<E, smallestLegal, highBound>::Read(const IPC::Message*, void**, IPC::TypedEnumSerializer<E, smallestLegal, highBound>::paramType*) [with E = mozilla::layers::GeckoContentController::APZStateChange; E smallestLegal = (mozilla::layers::GeckoContentController::APZStateChange)0u; E highBound = (mozilla::layers::GeckoContentController::APZStateChange)5u; IPC::TypedEnumSerializer<E, smallestLegal, highBound>::paramType = mozilla::layers::GeckoContentController::APZStateChange]'
  ipc/chromium/src/chrome/common/ipc_message_utils.h:121:41:   required from 'bool IPC::ReadParam(const IPC::Message*, void**, P*) [with P = mozilla::layers::GeckoContentController::APZStateChange]'
  obj-firefox/ipc/ipdl/_ipdlheaders/mozilla/dom/PBrowserChild.h:910:49:   required from 'bool mozilla::dom::PBrowserChild::Read(T*, const Message*, void**) [with T = mozilla::layers::GeckoContentController::APZStateChange; mozilla::dom::PBrowserChild::Message = IPC::Message]'
  obj-firefox/ipc/ipdl/PBrowserChild.cpp:2146:62:   required from here
  ipc/IPCMessageUtils.h:156:76: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]

	Botond Ballo [:botond] Assignee
	Comment 1 • 10 years ago

Patch that fixes the problem by specializing the range checking code to omit the lower bound check when the enumeration's underlying type is unsigned and the lower bound is 0.

Also asking for Ehsan's feedback because in my last conversation with him I claimed that 'IntType' in RangeChecker will be the ennumeration's underlying type (and thus an integer type), but that turned out to be wrong. It can also be an enumeration type, and I had to adjust the check in the default value of 'smallestLegalIsZero' to be 'smallestLegal == IntType(0)' for that reason.

	Botond Ballo [:botond] Assignee
	Comment 2 • 10 years ago

Comment on attachment 8405690 [details] [diff] [review]
bug995510.patch

This patch didn't fix the problem. Unflagging while I investigate.

	Botond Ballo [:botond] Assignee
	Comment 3 • 10 years ago

The problem is that on platforms where enum classes are supported, the check 'smallestLegal <= aValue' is done with the types of 'smallestLegal' and 'aValue' being the enumeration type, not the underlying type. My patch specializes this code for the case where the type of 'smallestLegal' is an unsigned type, but an enum type with an unsigned underlying type is not an unsigned type.

To specialize the code for the correct set of types (enums with an unsigned underlying type), we need to be able to derive the enum's underlying type from the enum type. To my knowledge this is impossible without compiler support (the C++11 library contains an std::underlying_type type trait, but its implementation requires compiler support). GCC only provides std::underlying_type and the compiler builtin used to implement it (__underlying_type) in 4.7 and higher [1] [2], but it supports typed enums (and we use that support) in 4.6 and higher [3].

I therefore cannot think of a good way of fixing this at the moment. If anyone has any ideas, I'd like to hear them. In the meantime, I will work around this problem in bug 976605 by changing the underlying type of my enumeration to be signed.

[1] http://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Type-Traits.html#Type-Traits
[2] http://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Type-Traits.html#Type-Traits
[3] http://gcc.gnu.org/projects/cxx0x.html

	Benoit Jacob [:bjacob] (mostly away)
	Comment 4 • 10 years ago

Hah, I hit the exact same problem in bug 987305. There are r+'d patches there, waiting on some other bugs to land, with a solution to this problem.

First of all, as far as I know, this overzealous warning is a long-time GCC bug, fixed in GCC 4.8. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=11856

Second, there is a simple way to silence it, that does not involve template trick: simply hide away the warning-triggering comparison in an inline function. In this way, even if the compiler inlines it and ends up evaluating this comparison at compile time, that will then be past the point where it would generate this warning. At least it fixed it for me on GCC 4.6.

See the patch on bug 987305, https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=987305&attachment=8398114 , specifically the IPCMessageUtils.h part. Search for IsLessThanOrEqual.

If it's blocking your work, you could try taking this bit from this patch, or we could consider landing this patch asap.

	Botond Ballo [:botond] Assignee
	Comment 5 • 10 years ago

(In reply to Benoit Jacob [:bjacob] from comment #4)
> Second, there is a simple way to silence it, that does not involve template
> trick: simply hide away the warning-triggering comparison in an inline
> function. In this way, even if the compiler inlines it and ends up
> evaluating this comparison at compile time, that will then be past the point
> where it would generate this warning. At least it fixed it for me on GCC 4.6.

Clever!

> If it's blocking your work, you could try taking this bit from this patch,
> or we could consider landing this patch asap.

I changed my enum's underlying type to be signed for now, so it's not blocking my work. I like the refactoring being done in that bug, though! What other bugs is it waiting on to land?

	Benoit Jacob [:bjacob] (mostly away)
	Comment 6 • 10 years ago

I've just landed the main part of bug 987305 now, which contains what is relevant here. I was waiting on something else to land the other part of bug 987305.