900134 - add MOZ_DEFAULT to Attributes.h

Status: RESOLVED WORKSFORME

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

Along the same lines as MOZ_DELETE, though perhaps not as useful

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: add MOZ_DEFAULT to Attributes.h

Some static constructor elimination patches are only doable if we have defaulted
function support in the compiler.  (The files in question have a base type with
a private no-op destructor, and derived types that also have private no-op
destructors.  Deleting the definitions would make the destructors public, which
I gather is undesirable.  Defaulting is exactly what we want here.)  Hence,
this patch.

I have tried to provide examples for when you would want to use this functionality
in the comments, but I'm short of examples.  And frankly, it's a little esoteric
knowing when this thing is useful in the first place (but very useful when you
need it).  I also considered something like:


but that didn't seem too much better.  WDYT?

Comment 2

[Joshua Cranmer [:jcranmer]]

I was trying to think of where this could be mocked, and it comes down to that this only works for no-arg constructors and destructors. Copy/move constructors/assignment operators could also be default'd, but they are not nontrivially writable.

It might make more sense to instead introduce a pair of macros, MOZ_TRIVIAL_CONSTRUCTOR and MOZ_TRIVIAL_DESTRUCTOR, that use = default where available. For example:

class PodClass {
  int someData;
private:
  MOZ_TRIVIAL_CONSTRUCTOR(PodClass)
  MOZ_TRIVIAL_DESTRUCTOR(PodClass)
};

This emphasizes why and where you could use this implementation safely. It also prevents you from doing something stupid, like defaulting a virtual destructor (virtual destructors are never trivial). Another idea would be to have the macro assert that the class with the trivial constructor/destructor is in fact trivially copyable (via type_traits).

[Note: you need a trivial destructor to be trivially-copyable. You need a trivial constructor and trivially-copyable to be trivial. You need to be trivial to be POD.]

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 783893 [details] [diff] [review]
add MOZ_DEFAULT to Attributes.h

Review of attachment 783893 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I dunno.  I once thought about adding this, and I thought I had a use case.  Then I thought more and either realized I didn't, or there was a workaround, or forgot about it entirely.

How about we say this is fine enough as long as it lands with someone using it, with there being no other way to get the same functionality?

::: mfbt/Attributes.h
@@ +182,5 @@
>  
>  
>  #ifdef __cplusplus
>  
> +/* MOZ_DEFAULT, specified immediate prior to the ';' terminating a declaration

/*
 * MOZ_DEFAULT...

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: make SMIL types have default constructors/destructors where possible

Waldo asked for an example patch where this could help; here is such a patch.

Making this change enables some compilers to elide static constructors and
destructors, since the compiler understands the semantics of default
constructors and destructors.

However, it is not absolutely necessary that we do it this way.  A lot of the
SMIL types have |static TYPE sSingleton| in some file.  We could change that
so that we had something like:

TYPE& Singleton()
{
  static TYPE singleton;
  return singleton;
}

and use calls to |Singleton| as appropriate.  The function-local singleton isn't
constructed until |Singleton| is called, which eliminates the static constructors
also.

I think the MOZ_DEFAULT approach is a little more elegant and also slightly more
efficient.  But we've done the |Singleton()| approach elsewhere; indeed, some of
the SMIL types have already had this treatment applied to them.

smaug suggested that dholbert knows the SMIL stuff the best.  So, asking for his
feedback on this approach vs. the alternative and even on MOZ_DEFAULT itself if
he feels so inclined.

Comment 5

[Daniel Holbert [:dholbert]]

I don't understand how MOZ_DEFAULT would help in the SMIL code - as far as I can tell, it's just a longer replacement for {}.

What am I missing?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #5)
> I don't understand how MOZ_DEFAULT would help in the SMIL code - as far as I
> can tell, it's just a longer replacement for {}.
> 
> What am I missing?

GCC understands that:

SMILType() = default;

does default constructor-y things, like initializing members to default values and so forth.  Of course, SMILType() doesn't have any members.  So GCC can tell in this case that it does nothing.  GCC cannot make the same determination about:

SMILType() {}

which means that when you have:

static SMILType sSingleton;

using |=default| means that GCC will optimize away the static constructor and destructor, since it can see they do nothing, whereas with |{}|, you get a static constructor and destructor.

Comment 7

[Mike Hommey [:glandium]]

In that particular case, MOZ_CONSTEXPR SMILType() {} would have the same effect.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #7)
> In that particular case, MOZ_CONSTEXPR SMILType() {} would have the same
> effect.

For the constructor, yes.  The SMIL types also happen to declare a private |~SMILType {}| destructor, which AFAICS (though I haven't actually tried running it through the compiler) cannot be declared constrexpr and would need |=default|.  (Or you could just not declare the destructor as long as the constructor is private, I suppose...)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> (Or you could just not declare the destructor as long as
> the constructor is private, I suppose...)

That would be fine with me.

They're only declared as private as a sanity-check, to be sure that no one can accidentally instantiate the class (beyond the singleton), or accidentally delete the singleton.

I think the accidental-instantiation situation would be a much more feasible mistak than accidental deletion.   So, I think I'd be fine not bothering with a destructor.

Comment 10

[Daniel Holbert [:dholbert]]

Or better, wrapping the destructor in #ifdef DEBUG, to get us the sanity-checking in debug builds, but without the overhead of a static destructor in opt builds.

Comment 11

[Nathan Froyd [:froydnj]]

We have |= default| support everywhere now, so this bug is moot.  The static constructor bits can be moved to a separate bug if they are still a problem.