Closed
Bug 900134
Opened 11 years ago
Closed 8 years ago
add MOZ_DEFAULT to Attributes.h
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: froydnj, Unassigned)
Details
Attachments
(2 files)
3.75 KB,
patch
|
Waldo
:
feedback+
|
Details | Diff | Splinter Review |
15.99 KB,
patch
|
Details | Diff | Splinter Review |
Along the same lines as MOZ_DELETE, though perhaps not as useful
Reporter | ||
Comment 1•11 years ago
|
||
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?
Attachment #783893 -
Flags: feedback?(jwalden+bmo)
Comment 2•11 years ago
|
||
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•11 years ago
|
||
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...
Attachment #783893 -
Flags: feedback?(jwalden+bmo) → feedback+
Reporter | ||
Comment 4•11 years ago
|
||
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.
Attachment #785059 -
Flags: feedback?(dholbert)
Comment 5•11 years ago
|
||
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?
Reporter | ||
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
In that particular case, MOZ_CONSTEXPR SMILType() {} would have the same effect.
Reporter | ||
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #785059 -
Flags: feedback?(dholbert)
Reporter | ||
Comment 11•8 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•