8781 - Need support for enums in XPIDL

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement, P3)

Description

[Alec Flett]

Just talking to mccabe on IRC - I need enum support for some messenger stuff...

I need:
- C++ generated enum code
- typelib support for enums (mccabe says they'll come out as constants)

looks like some of the code is already there, but it's not complete.

Comment 1

[John Bandhauer]

What is it exactly that want/need?

What we have is constants. These generate one off enums in the C++ header (in
class scope). And you get the name/value pair reflected into JS.

What you don't get is enums as types for parameters. We decided to not do this
for a good reason... C++ allows the C++ compiler to optimize the storage of
enums to fit their possible values. That means that for an enum with a small set
of values the compiler *might* use a small size holder, while for larger values
it might use some other size. This makes for a gray area in xptcall-like
mapping. We can't have that.

Also, the enums that the (Corba) compiler front end allow don't support setting
explicit values in the source. This sucks big time.

See my discussion of this in:
news://news.mozilla.org/36FEBE43.E1F8969D%40netscape.com
search for the phase:
"Added support in header and typelib output for constants"

Note that idl constants allow for inline math. Also, we can easily fo bitflags
like in
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/idl/nsIXPCSecurityManager
.idl#28

I strongly argue that idl enums should not be added.

Comment 2

[John Bandhauer]

added warren to cc list since he just called me and asked be about enums in
xpidl.

Comment 3

[Alec Flett]

hmm... so the concern is basically that the enum is of an unknown size so
XPConnecting it would be unreliable?

grr. I thought C garanteed that enums were ints, does C++ remove this
restriction?

Comment 4

[John Bandhauer]

ansi C spec (6.4.2.2) says: "Each enumerated type shall be compatible with an
integer type: the choice of type is implementation defined".

I don't have the C++ standard handy, but Stroustrup's the C++ Prog Lang 3rd.
says on p. 78: (and I paraphrase :) sizeof(some enum) <= sizeof(int), could be 1
or maybe 4 but not 8 on a machine where sizeof(int) == 4.

The real point is that the size is dependent on the enum's possible values and
the whims of the *particular* compiler used to compile the code. This is too
problematic for an xpcom interface (I argue). C++ compile-time enum legal value
checking is also a stretch to xpcom and ought not be counted upon. I think that
reliable component software ought to incur the cost of runtime checking of these
sorts of params. xpcom != C++

Comment 5

[Warren Harris]

I thought the only compiler that had a issue with this was Metrowerks, but there
are project settings where you can declare that enums are always 32-bit ints.

I'm sure win16 had problems with this too, but we don't care about that anymore.

All I can say is that it sure would be nice if enums were types in xpidl.

Comment 6

[Alec Flett]

Yes, and I'm not asking for XPCOM to range-check enums as they cross interfaces,
just that:
1) symbolic enum values are accessable from JS, and passable to JS-reflected
XPCOM interfaces
2) Generated C++ headers declare enums, rather than ints, to pass across
functions.

I'm going to bring this up on mozilla.xpcom with one more argument for why we
should not care about the C/C++ specification of enums.

Comment 7

[Mike McCabe]

Marking as assigned, to keep track until I can boil discussion of this into a
FAQ.

Comment 8

[Alec Flett]

well, I eventually decided to just go with interface consts.
But I agree that this should go into a FAQ

Comment 9

[Mike McCabe]

Done - jband answered this on http://www.mozilla.org/scriptable/faq.html.

Marking won't fix.

(and thanks for getting back to this and reminding me to resolve it.)

Comment 10

[Håkan Waara]

*** Bug 352485 has been marked as a duplicate of this bug. ***

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I would like to revisit this issue.  The use of ints here imposes an unnecessary lack of type-safety in code that uses IDLs.  C++11 allows you to specify the underlying type of an enum, like

  enum EFoo : PRInt32 { foo, bar, baz };

(See N2347, part 3.2: <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2347.pdf>.)  This is available in gcc since 4.4, Clang since 2.9, and VC++ for a long time, according to pages I found online.  Is there any compiler we care about that doesn't support this language feature, *and* doesn't default all enums to four bytes?  (In particular, I believe gcc historically defaults all enums to four bytes.)  If not, I don't think there's any good reason to give up type safety here for C++ code.  We can ensure that no one tries to use a different compiler by using a static assert on the sizeof the enum.

(We do want to make sure that the binding code for all languages ensures that the function is only called with correct enum values.  Otherwise people will probably write C++ functions that assume the values must be in the allowed set, and get very confused when they aren't.)

This might be nontrivial to do, mind you, and I'm not volunteering, but I don't think this should be WONTFIX at this point.

Comment 12

[Joshua Cranmer [:jcranmer]]

I would argue that you probably want enum classes (w/ underlying types). So the IDL:

enum nsFooValue {
  OptA,
  OptB
};

would become in C++ bindings:

MOZ_BEGIN_ENUM_CLASS(nsFooValue, uint32_t)
  OptA,
  OptB
MOZ_END_ENUM_CLASS(nsFooValue)

The XPT stuff could be generated as if all uses of nsFooValue were really unsigned long values (for everyone who supports enum classes, the ABI should be the same; the only question then is whether or not MSVC uses the same ABI in this case), and assuming that the nsFooValue were really defined like so:
interface nsFooValue {
  const unsigned long OptA = 0;
  const unsigned long OptB = 1;
};

This would allow JS people to use it via things like Components.interface.nsFooValue.OptA.

Comment 13

[Joshua Cranmer [:jcranmer]]

Attachment: Add enum support to xpidl via TypedEnum.h

I honestly have no idea who needs to review this, but this is a simple implementation based on my last comment. xpidl header.py generates enums using MFBT's MOZ_ENUM_CLASS support, and typelib.py generates it according to an interface with only const members. I've chosen int32_t as the underlying type, since that is the general baseline as far as ABI is concerned without unsigned constants.

Comment 14

[Joshua Cranmer [:jcranmer]]

Note that I'm not implementing the following features:
1. Support for limiting the enums to specific values. That requires invasive xpconnect and typelib modifications, which are well beyond my scope of knowledge.
2. Support for types other than int32_t. Underlying types aren't 100% supported on all platforms (the odd one out here is gcc 4.4). Also, our const support is in effect limited to 16 and 32 bit signed/unsigned types.
3. Support for specifying the value on an enumeration. This isn't hard to do in a follow-on, though.

The main purpose of this as it stands is to be able to replace interfaces that act as enum holders with actual enums, which would also help both the utility of warnings (we would pick up warnings on "not using all the enum types in a switch", e.g.) and reduce false positive (anonymous enum mismatches).

Comment 15

[Mike McCabe]

I'm very pleased to see that xpidl seems to have been rewritten in Python.

I'm amazed that this issue is (nearly) resolved after ALMOST 14 YEARS.

Comment 16

[Joshua Cranmer [:jcranmer]]

(In reply to Mike McCabe from comment #15)
> I'm very pleased to see that xpidl seems to have been rewritten in Python.
> 
> I'm amazed that this issue is (nearly) resolved after ALMOST 14 YEARS.

Because this issue was marked as WONTFIX for 13 of those years and was impractical to fix before C++11 came around.

Comment 17

[Benjamin Smedberg]

I probably won't be able to get to this review until after the perf workweek, so the week of 25-March.

Comment 18

[Benjamin Smedberg]

I think we should WONTFIX this (again). There's probably no reason to add new features to the XPCOM binding system at this point.

Comment 19

[Jean-Yves Perrier [:teoli]]

I removed the useless ddn as this has been WONTFIXed