Open Bug 239460 Opened 17 years ago Updated 11 years ago

Should xpidl convert constant variables to enums?

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set
normal

Tracking

()

People

(Reporter: tenthumbs, Assigned: dbradley)

Details

Consider this warning from my Linux build.

nsDOMClassInfo.cpp:5210: warning: enumeral mismatch in conditional 
expression: `nsIXPCSecurityManager::<anonymous enum>' vs 
`nsIXPCSecurityManager::<anonymous enum>'

The code in question is:

5207   rv = doCheckPropertyAccess(cx, obj, id, wrapper,
5208                              (flags & JSRESOLVE_ASSIGNING) ?
5209                              nsIXPCSecurityManager::ACCESS_SET_PROPERTY :
5210                              nsIXPCSecurityManager::ACCESS_GET_PROPERTY,
5211                              PR_FALSE);

In js/src/xpconnect/idl/nsIXPCSecurityManager.idl there is:

 99      const PRUint32 ACCESS_GET_PROPERTY = 1;
100      const PRUint32 ACCESS_SET_PROPERTY = 2;

but in js/src/xpconnect/idl/_xpidlgen/nsIXPCSecurityManager.h it's changed to

  enum { ACCESS_GET_PROPERTY = 1U };
  enum { ACCESS_SET_PROPERTY = 2U };

So clearly related constants appear in different enums and gcc is mildly upset.
Who knows what some future gcc might do.

This is just one example. Search the make output for others.

The question I have is whether xpidl should do this since this is changing the
semantics of the code.

The code in in xpidl_header.c:do_const_dcl.
(In reply to comment #0)
> The question I have is whether xpidl should do this since this is changing the
> semantics of the code.

Um, what else do you suggest?
Ideally they should be declared as const members, but I'm sure that will cause
grief with some older compilers. Newer compilers are going to be noisy because
they're more strict on type checking. I doubt this would ever become an error,
and at this point it's probably better to allow for older compilers than quiet
newer ones. I'm not sure this would ever be considered an error since enums can
be promoted to ints, it's the other way around that currently is an error
without casting help IIRC.

So my initial response is WONTFIX, but I'll wait for some counter arguments.

To quiet the warning and not break older compilers you could cast them to int on
use, but you'd have to do that for every occurance. That's probably not real fun
either.
I dont think a compiler that doesn't understand const will compile mozilla any 
way. I did a quick search with
  find mozilla -name \*.cpp |xargs grep -HI 'const.*int'
and turned up a large number of "const int foo = <explicit_number>" lines.

Yes, compilers are noisy but they do turn up things that need fixing. At one
time the configure scripts didn't include -W for gcc because it was deemed
"too noisy." But it turned up a number of real problems. Besides warnings
often turn into errors. Consider how many things gcc-2.95.3 accepts with a
warning that gcc-3.3.3 considers an error. Introducing warnings seems risky in 
the long run.
well, shaver confirmed that static const class members weren't always in the C++
standard. does MSVC 6 support them? (g++ 2.95 does, I checked)
Yes, VC++ 6 supports them, and I think it supports regular const members as
well. It's not an issue of supporting const. It's an issue of supporting const
members which is something different. const members, whether they are static or
not, haven't been around a real long time, at least when you consider the
timespan Mozilla's compilers were written.
(In reply to comment #5)
> Yes, VC++ 6 supports them

Hmm, apparently that's not true. bz checked in a patch that used them (bug
239079), and all msvc6 tinderboxes turned red.

this makes this bug wontfix, I guess.
Actually, tbox went red correctly; I initialized the members in the class decl,
which is a no-no....  I just checked in the bustage fix.
Whether there are const members or not, it seems to me if you want to ban some 
feature for backward compatibility you should do just that and not silently 
change the program, e.g. exceptions and rtti.
IIRC, VC6 will not let you initialize static const ints in the class definition.

That doesn't mean this should be WONTFIX. It's one more reason that it's high
time mozilla.org upgraded to a more recent Microsoft compiler.
Never mind compiler *support*, but will the code be as efficient?
(I filed an alternative proposal for fixing the warning as bug 262000.)
Of course it would be as efficient. These are compile-time constants.
It's not just a VC++ 6 issue, you have to be concerned about some other obscure 
platforms where the compilers haven't been updated. I'm not completely sold on 
that, but that's what I've encountered when I've looked into similar compiler 
support issues.
Component: xpidl → XPCOM
QA Contact: pschwartau → xpcom
Converting the constant to enum is ignoring the type specified in the idl, which could cause some surprises.  The enumerators have their own type and even their integral promotion type is unrelated to the type specified in the idl.

e.g. "const PRUint32 ACCESS_GET_PROPERTY = 1" is being converted to
     "enum { ACCESS_GET_PROPERTY = 1U }" and the type of the integral
     promotion of ACCESS_GET_PROPERTY is implementation defined,
     but with g++ it is signed int.
Bug 8781 had proposed adding enum support to xpidl. But that was won't fixed. Reason was given here http://www.mozilla.org/scriptable/faq.html#i2.
You need to log in before you can comment on or make changes to this bug.