fix compilation problems Web IDL enums and GCC 4.4

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla23
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

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

Comment 2

6 years ago
...and another similar patch.
Attachment #745950 - Flags: review?(ehsan)
Reporter

Updated

6 years ago
Blocks: webaudio

Comment 3

6 years ago
Can't we fix the Web IDL codegen to generate something that gcc 4.4 can understand?
Flags: needinfo?(bzbarsky)
Reporter

Comment 4

6 years ago
(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

?
Flags: needinfo?(ehsan)

Comment 5

6 years ago
(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.
Flags: needinfo?(ehsan)

Updated

6 years ago
Blocks: ParisBindings
No longer blocks: webaudio
Component: Video/Audio → DOM
Summary: fix compilation problems with dom::{ChannelCountMode,ChannelInterpretation} and GCC 4.4 → fix compilation problems Web IDL enums and GCC 4.4
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.
Flags: needinfo?(bzbarsky)

Updated

6 years ago
Attachment #745942 - Flags: review?(ehsan) → review-

Updated

6 years ago
Attachment #745950 - Flags: review?(ehsan) → review-

Comment 7

6 years ago
(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?

Comment 11

6 years ago
(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.  ;)

Comment 13

6 years ago
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?
Reporter

Comment 15

6 years ago
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.
Attachment #745942 - Attachment is obsolete: true
Attachment #745950 - Attachment is obsolete: true
Attachment #746007 - Flags: review?(bzbarsky)

Comment 16

6 years ago
(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...
Reporter

Comment 17

6 years ago
(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.

Comment 18

6 years ago
(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.
Reporter

Comment 19

6 years ago
(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?  :(
Reporter

Comment 21

6 years ago
(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.
Reporter

Comment 22

6 years ago
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.
Attachment #746067 - Flags: review?(bzbarsky)
Reporter

Updated

6 years ago
Attachment #746007 - Attachment is obsolete: true
Attachment #746007 - Flags: review?(bzbarsky)
Comment on attachment 746067 [details] [diff] [review]
make WebIDL enums enum classes instead of plain enums

r=me
Attachment #746067 - Flags: review?(bzbarsky) → review+
Reporter

Comment 24

6 years ago
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?
Reporter

Comment 26

6 years ago
(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.  :(
https://hg.mozilla.org/mozilla-central/rev/87a68fd84800
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 29

6 years ago
(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.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.