623123 - Add constructor for nsXULAppInfo (which inherits from nsIXULAppInfo) to placate CLang

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

nsXULAppInfo is missing a user defined constructor,
but in nsAppRunner.cpp a const variable of this type is defined.

This is not valid c++. For more information see "Default initialization of
const variable of a class type requires user-defined default constructor" in
http://clang.llvm.org/compatibility.html#c++

Comment 1

[timeless]

The reporter's summary and initial comment were both lame. I'm merely adjusting the summary and providing a better link. I am not passing judgement on the quality of the bug report.

http://clang.llvm.org/compatibility.html#default_init_const

Comment 2

[Benjamin Smedberg]

The constructor was omitted on purpose so that the type is POD-initialized instead of requiring a static initializer function.

I'm not sure about this rule: is it all class types that require a constructor, even POD types? What if you use a static initializer of the form:

struct Foo {
  int a;
  int b;
};

const Foo foo = { 1, 2 };

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> The constructor was omitted on purpose so that the type is POD-initialized
> instead of requiring a static initializer function.
> 
> I'm not sure about this rule: is it all class types that require a constructor,
> even POD types? What if you use a static initializer of the form:
> 
> struct Foo {
>   int a;
>   int b;
> };
> 
> const Foo foo = { 1, 2 };

AFAIK, this is allowed (thankfully). And IIRC, C++0x also extends cases where this is allowed.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Yes,

struct Foo {
  int a;
  int b;
};
const Foo foo = { 1, 2 };

is OK, but 

struct Foo {
  int a;
  int b;
  virtual void bar();
};
const Foo foo = { 1, 2 };

is not and NS_DECL_ISUPPORTS_INHERITED expands to virtual methods.

From the standard:
[class] p4

A POD-struct is an aggregate class that has no non-static data members of type non-POD-struct, non-POD-union (or array of such types) or reference, and has no user-declared copy assignment operator and no user-declared destructor.

[dcl.init.aggr]

An aggregate is an array or a class (clause 9) with no user-declared constructors (12.1), no private or protected non-static data members (clause 11), no base classes (clause 10), and no virtual functions (10.3).

Comment 5

[Benjamin Smedberg]

This is only acceptable if it does not cause either GCC or clang to emit/call a static-initializer function. I'm pretty sure that at least with GCC 4.3 a static initializer function will be called even for a trivial do-nothing constructor.

I've said before on various lists that I believe the C++ standard's definition of POD is too strict, and written patches to disable the pedantic offsetof-non-POD warning in GCC. I believe clang should be taught accept this construct, perhaps with a special permissive flag.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: assembly produced by gcc 4.2 with this patch

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: assembly produced by gcc 4.2 without this patch

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: assembly produced by clang with this patch

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I just attached the assembly files for nsAppRunner.cpp.

$ grep nsXULAppInfo.*C1 *.s
$ grep nsXULAppInfo.*C2 *.s

So it looks like your concern is not a problem. However, I must say I am a bit disturbed by the comment. I respect your opinion, but find it surprising that it should take precedence over the C++ standard, specially when that means we wouldn't be able to support xcode4.

Comment 10

[(dormant account)]

(In reply to comment #9)
> I just attached the assembly files for nsAppRunner.cpp.
> 
> $ grep nsXULAppInfo.*C1 *.s
> $ grep nsXULAppInfo.*C2 *.s
> 
> So it looks like your concern is not a problem. However, I must say I am a bit
> disturbed by the comment. I respect your opinion, but find it surprising that
> it should take precedence over the C++ standard, specially when that means we
> wouldn't be able to support xcode4.

We are concerned that C++ sometimes forces perf hits on us. Given a choice between taking a perf hit and respecting the C++ standard, we'd rather stay on the perf side. Luckily, sounds like it's not a problem in this case.

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Well, there are some things to keep in mind

*) There will not be a gcc 4.3 on os X and I wouldn't be surprised if the 4.2 support doesn't last all that long.
*) No one likes static initializes. Clang and llvm themselves just got rid of most of the ones they had. Not declaring a constructor and hoping the compiler can handle is just not the best way to do it.
*) There is more to performance then what shows up in a bug or two. I am doing this not because I am a language lawyer or because I think OS X is the best thing ever. Is because I want to try to optimize Firefox. With LLVM we would have easy access to most of the toolchain (it includes an assembler and hopefully will have a linker soon).

Comment 12

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Ping?
I think the concerns about static initializes are explained in

http://blog.mozilla.com/respindola/2011/01/26/clang_and_firefox_p2/

Let me know if there is anything else.

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/570781a55419