903543 - eliminate static constructors from SMIL types

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: eliminate static constructors from SMIL types

As discussed on IRC and elsewhere.

We could eliminate the Singleton() accessors that bug 877886 added now, but that'd
be a minor size optimization and best left for another bug.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #1)
> As discussed on IRC and elsewhere.

(for reference, elsewhere includes bug 900134 comment 7 through 10)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 788284 [details] [diff] [review]
eliminate static constructors from SMIL types

># HG changeset patch
># User Nathan Froyd <[e-mail]>
>
>eliminate static constructors from SMIL types

You should mention the destructors as well, in the commit message.

e.g.: "eliminate static constructors (and destructors, in opt builds)"

>+  MOZ_CONSTEXPR SMILBoolType() {}
>+#ifdef DEBUG
>   ~SMILBoolType() {}
>+#endif

It'd be nice to document the purpose of these #ifdef DEBUG wrappers. Maybe as part of this next comment here, in nsISMILType.h (the parent class of all the other ones here):

>diff --git a/content/smil/nsISMILType.h b/content/smil/nsISMILType.h
>   /**
>    * Protected destructor, to ensure that no one accidentally deletes an
>    * instance of this class.
>    * (The only instances in existence should be singletons - one per subclass.)
>    */
>+#ifdef DEBUG
>   ~nsISMILType() {}
>+#endif

Other than that, looks good!

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/00fc7e525678 for the incomprehensible https://tbpl.mozilla.org/php/getParsedLog.php?id=26378412&tree=Mozilla-Inbound

Comment 5

[Nathan Froyd [:froydnj]]

OK, so the error message was incomprehensible, but it has a reasonably simple explanation:

https://blog.mozilla.org/nfroyd/2013/08/12/the-language-lawyer-the-curious-constexpr-conundrum/

The upshot is that we can't define destructors if we are going to have constexpr constructors.

I think the next-best solution, then, is to only MOZ_CONSTEXPR the constructors in opt builds and only define the destructors in debug builds.  It's fiddly, but one must bow to the whims of the language gods.  WDYT?

Comment 6

[Daniel Holbert [:dholbert]]

Thanks for the explanation.

I think I lean towards fixing this by just dropping the protected destructors. I don't think there's really any risk of someone accidentally deleting one of these singletons without noticing.  (particularly given that you can only get at them by invoking "Singleton()", and you can't declare any additional ones).

IMHO, that risk is low enough that it doesn't really merit the maintainability / readability burden of having an awkwardly-#ifdeffed constructor & destructor in each of these classes.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: eliminate static constructors from SMIL types (and destructors, in opt builds)

OK, here's a constexpr patch that deletes the no-op destructors and therefore
should work with clang.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 790163 [details] [diff] [review]
eliminate static constructors from SMIL types (and destructors, in opt builds)

># HG changeset patch
># User Nathan Froyd <[e-mail]>
>
>Bug 903543 - eliminate static constructors from SMIL types (and destructors, in opt builds)
>
>OK, here's a constexpr patch that deletes the no-op destructors and therefore
>should work with clang.

I'm guessing you didn't want that "OK, here's ..." to end up inside the patch. :) (Right now, I think it's part of the extended commit message.)  So, probably delete that before landing.

Also, please update the first line of the commit message to indicate that you're removing the destructors entirely now, and maybe mention MOZ_CONSTEXPR for the constructors.

r=me with that.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ea63efc27c2