262000 - xpidl should emit adjacent consts of the same type in the same enum

Status: RESOLVED DUPLICATE of bug 1498059

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think xpidl should emit adjacent consts of the same time in the same anonymous
enum rather than in separate ones.  This is most likely to make gcc's enumeral
mismatch warnings useful.

Actual results:

 <objdir>/docshell/base/nsIScrollable.h contains (with extra blank lines
 removed):

  enum { ScrollOrientation_X = 1 };                                            
                                   
  enum { ScrollOrientation_Y = 2 };
 ...
  enum { Scrollbar_Auto = 1 }; 
  enum { Scrollbar_Never = 2 }; 
  enum { Scrollbar_Always = 3 };

Expected results:
 <objdir>/docshell/base/nsIScrollable.h contains (with extra blank lines
 removed):

  enum {
    ScrollOrientation_X = 1,                                             
    ScrollOrientation_Y = 2
  };
 ...
  enum {
    Scrollbar_Auto = 1,
    Scrollbar_Never = 2,
    Scrollbar_Always = 3
  };

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

s/of the same time/of the same type/

Comment 2

[David Bradley]

Yes, I agree, I wondered why enum's weren't supported in IDL.

One issue is, that enum's aren't guarenteed to be of size int. That might cause
problems for some frozen interfaces.

Comment 3

[Braden]

How is the size of a compile-time constant going to affect frozen interfaces?

This isn't really related to enum support in XPIDL. It's a slightly different
mapping of XPIDL consts from the current one. IMO, the distinction you're
attempting to draw is just too subtle to justify the generation of different
code. It defies the principle of least surprise.

Comment 4

[David Bradley]

I was thinking the method's arguments would be defined as the enum type. I 
probably went off track with that assumption. If we're just talking using the 
enum to define the constants and not the argument types it's not an issue.

Comment 5

[Braden]

The enums in question here are anonymous; so the type isn't available to be used
in an argument list.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Patch

Here's my shot at it.  Output for nsITypeAheadFind.h with my patch:

  enum { FIND_FOUND = 0U,
         FIND_NOTFOUND = 1U,
         FIND_WRAPPED = 2U };

Comment 8

[Josh Matthews [:jdm]]

Attachment: Patch

Removed some cruft.

Comment 9

[David Bradley]

Looking at the patch now.

Comment 10

[David Bradley]

Tried building with the patch, but I'm not seeing the changes. Trying to figure out if my build is getting a hold of a pre-built xpidl or something.

Comment 11

[ISHIKAWA, Chiaki]

(In reply to comment #8)
> Created attachment 441520 [details] [diff] [review]
> Patch
> 
> Removed some cruft.

The patch worked beautifully on my linux build configuration to compile TB3.
(I used relatively recent comm-central, about a week old.)

After applying the patch to xidl_header.c, I recompiled TB3.
I looked for the tell-tale "anonymous" string
in the compilation log, and only found one unrelated warning. 
We eliminated about 40+ (or 50-) warning messages with this single patch from TB3 compilation(!)
Wonderful.

TIA

Comment 12

[Ludovic Hirlimann [:Usul]]

(In reply to comment #10)
> Tried building with the patch, but I'm not seeing the changes. Trying to figure
> out if my build is getting a hold of a pre-built xpidl or something.

Did you try cloberring ?

Comment 13

[Josh Matthews [:jdm]]

This code has been replaced by pyxpidl.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Re-closing this as a duplicate of bug 1498059, where we actually do something similar to this is pyxpidl now.