GCC 4.4 doesn't like $TYPE::enumValue; it really wants $NAMESPACE::enumValue.

...and another similar patch.

Can't we fix the Web IDL codegen to generate something that gcc 4.4 can understand?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3) > Can't we fix the Web IDL codegen to generate something that gcc 4.4 can > understand? This question confuses me. Codegen generates: namespace ChannelInterpretationValues { enum valuelist { Speakers, Discrete }; extern const EnumEntry strings[3]; } // namespace ChannelInterpretationValues typedef ChannelInterpretationValues::valuelist ChannelInterpretation; Are you saying you'd rather see something like: namespace ChannelInterpretation { enum valuelist { Speakers, Discrete }; extern const EnumEntry strings[3]; } // namespace ChannelInterpretation ?

(In reply to comment #4) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #3) > > Can't we fix the Web IDL codegen to generate something that gcc 4.4 can > > understand? > > This question confuses me. Codegen generates: > > namespace ChannelInterpretationValues { > > enum valuelist { > Speakers, > Discrete > }; > > extern const EnumEntry strings[3]; > } // namespace ChannelInterpretationValues > > typedef ChannelInterpretationValues::valuelist ChannelInterpretation; > > Are you saying you'd rather see something like: > > namespace ChannelInterpretation { > > enum valuelist { > Speakers, > Discrete > }; > > extern const EnumEntry strings[3]; > } // namespace ChannelInterpretation > > ? No, what I would like to see is the exact current code for all non-buggy compilers, and something like you suggest for the buggy compilers. I have no interest in modifying every call site that uses Web IDL enums to work around gcc's stupidity.

The way codegen is expected to be used, last I checked is that the C++ enum value is ChannelInterpretationValues::Speakers and the like. That said, if we could make it so that ChannelInterpretation is an enum type _and_ ChannelInterpretation::Speakers is a value of that enum, that would be ideal.

(In reply to Boris Zbarsky (:bz) from comment #6) > The way codegen is expected to be used, last I checked is that the C++ enum > value is ChannelInterpretationValues::Speakers and the like. Oh really? That seems suboptimal, and very ugly. > That said, if we could make it so that ChannelInterpretation is an enum type > _and_ ChannelInterpretation::Speakers is a value of that enum, that would be > ideal. You can't do that with regular enums, but perhaps Web IDL can generate enum classes?

And in particular, current code in DOM uses FooEnumValues::Whatever. https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Enumeration_types documents that clearly. Again, if there is a way to have the enum type be a namespace for the values, so much the better. That's just not how enums work in C++. :(

> That seems suboptimal, and very ugly. Well, the options are for the value namespace to not match the webidl enum name or for the enum type to not match it. Modulo enum classes, yes. If we have support for those across the board now, we could do it....

But enum classes won't let you cast integers to the enum, will it? Or will the constructor for the enum class from the !MOZ_HAVE_CXX11_STRONG_ENUMS case in TypedEnum.h save the day?

(In reply to comment #9) > > That seems suboptimal, and very ugly. > > Well, the options are for the value namespace to not match the webidl enum name > or for the enum type to not match it. > > Modulo enum classes, yes. If we have support for those across the board now, > we could do it.... We have support for enum classes where they're available and emulate them where there are not. The only limitation that I'm currently aware of is that they cannot be used inside C++ classes, but that's something that can easily be fixed, and won't be a problem in this case. (In reply to comment #10) > But enum classes won't let you cast integers to the enum, will it? They won't, and that's a feature! :-) > Or will > the constructor for the enum class from the !MOZ_HAVE_CXX11_STRONG_ENUMS case > in TypedEnum.h save the day? Not sure what this question means.

> They won't, and that's a feature! :-) This code needs to work: 425 XMLHttpRequestResponseType ResponseType() 426 { 427 return XMLHttpRequestResponseType(mResponseType); 428 } where ResponseTypeEnum is some enum in XHR code. Likewise, for this code: XMLHttpRequestResponseType arg0; int index = FindEnumStringIndex<false>(cx, argv[0], XMLHttpRequestResponseTypeValues::strings, "XMLHttpRequestResponseType", &ok); arg0 = static_cast<XMLHttpRequestResponseType>(index); or some variants thereof. I'm open to reasonable changes to code like that, but big conversion tables are not reasonable. ;)

Oh I thought you're asking about implicit conversions. Explicit conversions do work as expected, so I think this won't be a problem.

Do you know whether we have boxes both with and without MOZ_HAVE_CXX11_STRONG_ENUMS on try?

Here's a patch that makes codegen produce: MOZ_BEGIN_ENUM_CLASS(XMLHttpRequestResponseType, uint32_t) _empty, Arraybuffer, Blob, Document, Json, Text, Moz_chunked_text, Moz_chunked_arraybuffer, Moz_blob MOZ_END_ENUM_CLASS(XMLHttpRequestResponseType) namespace XMLHttpRequestResponseTypeValues { extern const EnumEntry strings[10]; } // namespace XMLHttpRequestResponseTypeValues and I think might make everybody happy in terms of code cleanliness. The code conversion was straightforward, but there are a couple of places that had to change because WebIDL enums are no longer integers: * nsXMLHttpRequest is slightly uglified. If there's a better way to do this, I'd be happy to hear about it; * AudioNodeStream can no longer have bitfield WebIDL enum members; * FileHandle::Open had to be tweaked because we can't convert the enum to some other random enum.

(In reply to comment #14) > Do you know whether we have boxes both with and without > MOZ_HAVE_CXX11_STRONG_ENUMS on try? IIRC we do, since I broke some builds when I used them in a nested C++ class...

(In reply to Boris Zbarsky (:bz) from comment #14) > Do you know whether we have boxes both with and without > MOZ_HAVE_CXX11_STRONG_ENUMS on try? If I understand the bits in TypedEnums and _MSC_VER definitions correctly, our buildbot Windows boxes define MOZ_HAVE_CXX11_ENUM_TYPE but not MOZ_HAVE_CXX11_STRONG_ENUMS. Mac and Linux buildbots define both of those macros.

(In reply to comment #17) > (In reply to Boris Zbarsky (:bz) from comment #14) > > Do you know whether we have boxes both with and without > > MOZ_HAVE_CXX11_STRONG_ENUMS on try? > > If I understand the bits in TypedEnums and _MSC_VER definitions correctly, our > buildbot Windows boxes define MOZ_HAVE_CXX11_ENUM_TYPE but not > MOZ_HAVE_CXX11_STRONG_ENUMS. Mac and Linux buildbots define both of those > macros. That rings a bell. BTW, I wonder, we use gcc 4.4 for b2g, right? That should not support enum classes either.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18) > That rings a bell. BTW, I wonder, we use gcc 4.4 for b2g, right? That > should not support enum classes either. That's correct, on both counts.

> * nsXMLHttpRequest is slightly uglified. Yeah, I was worried about that. :( Is there really no way to explicitly convert one of these enum things to an int? :(

(In reply to Boris Zbarsky (:bz) from comment #20) > > * nsXMLHttpRequest is slightly uglified. > > Yeah, I was worried about that. :( > > Is there really no way to explicitly convert one of these enum things to an > int? :( static_cast seems to work. I will go back and fix things up.

Here is a somewhat less ugly version, depending on your opinion of static_cast. Works locally, but earlier versions had problems on MSVC, so waiting for a try run before committing this.

Comment on attachment 746067 [details] [diff] [review] make WebIDL enums enum classes instead of plain enums r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/87a68fd84800 I had to add a few dom:: prefixes here and there to make Android builds happy. If that winds up becoming a pervasive problem, we may need to tweak TypedEnum.h for our GCC 4.6.x.

I think that's a serious problem. Having code that people will commonly write and that compiles everywhere but Android is bad. We want these binding things to be usable, not footguns... So I think we need to deal with this issue. Why is it even coming up?

(In reply to Boris Zbarsky (:bz) from comment #25) > I think that's a serious problem. Having code that people will commonly > write and that compiles everywhere but Android is bad. We want these > binding things to be usable, not footguns... So I think we need to deal > with this issue. Why is it even coming up? That's a good question. I needed to dom::-prefix VisibilityState and BinaryType, but nothing else. My first guess is that those are somehow exported in Android headers, but developer.android.com turns up nothing along those lines. All my other guesses just sound like crazy talk. But the error sounds insane too: ../../../../content/base/src/nsDocument.cpp: In constructor 'nsIDocument::nsIDocument()': ../../../../content/base/src/nsDocument.cpp:1348:22: error: 'VisibilityState' is not a class, namespace, or enumeration ../../../../content/base/src/nsDocument.cpp: In member function 'mozilla::dom::VisibilityState nsDocument::GetVisibilityState() const': ../../../../content/base/src/nsDocument.cpp:10964:12: error: 'VisibilityState' is not a class, namespace, or enumeration ../../../../content/base/src/nsDocument.cpp:10967:10: error: 'VisibilityState' is not a class, namespace, or enumeration

There is a .visibilityState getter on the Document interface which becomes a VisibilityState method on nsIDocument. Presumably that's the source of the name collision there... Similarly, the WebSocket interface has a .binaryType getter. This is a fundamental problem with skipping Get on infallible getters and upcasing the first letter. :( I'm not quite sure what we can best do about it... And I'm not sure why different compilers disagree on how to handle it. :(

(In reply to comment #27) > This is a fundamental problem with skipping Get on infallible getters and > upcasing the first letter. :( I'm not quite sure what we can best do about > it... And I'm not sure why different compilers disagree on how to handle it. > :( FWIW clang rejects such code for me locally and I believe that is the correct behavior for a C++ compiler, but apparently C++ name lookup is recently figured out rocket science.

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Enumeration_types