Open Bug 239460 Opened 20 years ago Updated 1 year ago

Use const static members for XPIDL constants instead of anonymous enums

Categories

(Core :: XPCOM, task, P5)

Product:
Core
Component:
XPCOM
Platform:
x86
Linux
Type:
task

Tracking

()

People

(Reporter: tenthumbs, Unassigned)

References

Details

Attachments

(1 obsolete file) 

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.

The bug assignee didn't login in Bugzilla in the last 7 months.
:nika, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: dbradley → nobody
Flags: needinfo?(nika)

Before this change, all XPIDL constants were declared using an anonymous
enum rather than using a static constant. This change makes the
generated code more consistent with what is done in languages like Rust.

Some small changes were needed due to signed/unsigned comparison
warnings which were previously silent.

Assignee: nobody → nika
Status: NEW → ASSIGNED

This is easy enough to change, and seems worth doing. The compiler support issues are definitely not relevant 18 years later.

Flags: needinfo?(nika)
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9cc08c2f20be
Specify types for XPIDL consts in C++, r=xpcom-reviewers,mccr8

Backed out changeset 9cc08c2f20be (bug 239460) for causing build bustage.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a2aa25965e8b7d0450fa21cf9635306e31745dd6

Push with failures

Failure log

 In file included from Unified_cpp_dom_media_autoplay0.cpp:2:
[task 2022-04-06T23:54:36.069Z] 23:54:36     INFO -  /builds/worker/checkouts/gecko/dom/media/autoplay/AutoplayPolicy.cpp(112,7): error: call to 'GetInt' is ambiguous
[task 2022-04-06T23:54:36.069Z] 23:54:36     INFO -        Preferences::GetInt("media.autoplay.default", nsIAutoplay::ALLOWED);
[task 2022-04-06T23:54:36.069Z] 23:54:36     INFO -        ^~~~~~~~~~~~~~~~~~~
[task 2022-04-06T23:54:36.071Z] 23:54:36     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Preferences.h(140,19): note: candidate function
[task 2022-04-06T23:54:36.071Z] 23:54:36     INFO -    static nsresult GetInt(const char* aPrefName, int32_t* aResult,
[task 2022-04-06T23:54:36.071Z] 23:54:36     INFO -                    ^
[task 2022-04-06T23:54:36.071Z] 23:54:36     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Preferences.h(166,18): note: candidate function
[task 2022-04-06T23:54:36.071Z] 23:54:36     INFO -    static int32_t GetInt(const char* aPrefName, int32_t aFallback = 0,
[task 2022-04-06T23:54:36.071Z] 23:54:36     INFO -                   ^
[task 2022-04-06T23:54:36.071Z] 23:54:36     INFO -  1 error generated.
Flags: needinfo?(nika)

This reveals a fun new thing about MSVC/clang-cl which I hadn't realized until now: unlike other compilers, static integers like static constexpr int X = 0; can coerce to a null T* if their constant value happens to be 0 in these compilers, making the call above which got me backed out ambiguous. I can't think of any way to avoid this off the top of my head (it happens even if the expression isn't a literal 0 constant).

This isn't the case for any other compiler, and the fact that constants declared with enum { X = 0 }; don't have this property makes them startlingly less surprising to use here :-/

I think I'm going to abandon this patch for now, because I imagine there will be other failure cases like that, and I'm not sure if it's worth fixing them, as this change doesn't really have an immediate benefit other than generated code feeling slightly more modern & slightly better compiler warnings.

Assignee: nika → nobody
Status: ASSIGNED → NEW
Type: defect → task
Flags: needinfo?(nika)
Priority: -- → P5
Summary: Should xpidl convert constant variables to enums? → Use const static members for XPIDL constants instead of anonymous enums
Attachment #9271148 - Attachment is obsolete: true

It may be worth trying const instead of constexpr?

It behaves the same way as constexpr on msvc: https://godbolt.org/z/a8nGrePfh

(we don't really care about msvc anymore)

(In reply to Mike Hommey [:glandium] from comment #24)

(we don't really care about msvc anymore)

This is true, but clang-cl mimics this behaviour for compatibility with msvc, and we do use clang-cl.

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: