Last Comment Bug 8781 - Need support for enums in XPIDL
: Need support for enums in XPIDL
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: P3 enhancement (vote)
: ---
Assigned To: Joshua Cranmer [:jcranmer]
:
:
Mentors:
: 352485 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 1999-06-23 13:22 PDT by Alec Flett
Modified: 2015-09-26 14:16 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add enum support to xpidl via TypedEnum.h (20.97 KB, patch)
2013-03-09 22:03 PST, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review

Description Alec Flett 1999-06-23 13:22:38 PDT
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 1999-06-23 13:39:59 PDT
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 1999-06-23 13:40:59 PDT
added warren to cc list since he just called me and asked be about enums in
xpidl.
Comment 3 Alec Flett 1999-06-23 23:56:59 PDT
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 1999-06-24 00:34:59 PDT
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 1999-06-24 11:26:59 PDT
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 1999-06-24 11:36:59 PDT
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 1999-07-16 18:58:59 PDT
Marking as assigned, to keep track until I can boil discussion of this into a
FAQ.
Comment 8 Alec Flett 1999-07-26 16:29:59 PDT
well, I eventually decided to just go with interface consts.
But I agree that this should go into a FAQ
Comment 9 Mike McCabe 1999-07-26 17:11:59 PDT
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 2006-09-13 08:28:48 PDT
*** Bug 352485 has been marked as a duplicate of this bug. ***
Comment 11 Aryeh Gregor (:ayg) (away until October 25) 2012-07-20 05:19:40 PDT
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] 2012-08-09 07:09:21 PDT
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] 2013-03-09 22:03:48 PST
Created attachment 723156 [details] [diff] [review]
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] 2013-03-09 22:16:12 PST
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 2013-03-11 11:03:56 PDT
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] 2013-03-11 11:08:28 PDT
(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 [:bsmedberg] 2013-03-15 10:52:29 PDT
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 [:bsmedberg] 2015-04-20 06:59:29 PDT
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] 2015-09-26 14:16:39 PDT
I removed the useless ddn as this has been WONTFIXed

Note You need to log in before you can comment on or make changes to this bug.