895322 - Replace MOZ_STATIC_ASSERT with static_assert

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Masatoshi Kimura [:emk]]

After bug 894242, static_assert should be available everywhere.

Comment 1

[(no longer active)]

Attachment: static_assert.sh, the driver script

Comment 2

[(no longer active)]

Attachment: assert_replacer.py, the replacer engine

This script helps preserve indentation across multiple lines.

Comment 3

[(no longer active)]

Comment on attachment 777842 [details]
assert_replacer.py, the replacer engine

Hmm, this script has a bug.

Comment 4

[(no longer active)]

Attachment: static_assert.sh, the driver script

Comment 5

[(no longer active)]

Attachment: assert_replacer.py, the replacer engine

Comment 6

[(no longer active)]

Hmm, we seem to have one usage of MOZ_STATIC_ASSERT in xpcom/base/nsErrorAssertsC.c which was added in bug 836438.  Is it worth to keep MOZ_STATIC_ASSERT around for that?

Comment 7

[(no longer active)]

Hmm, there is another C usage in media/webrtc (the static asserts in media/webrtc/signaling/src/sipcc/core/includes/ccapi.h)

Comment 8

[Justin Lebar (not reading bugmail)]

Maybe we define MOZ_STATIC_ASSERT only ifndef __cplusplus?

Comment 9

[Joshua Cranmer [:jcranmer]]

There's also still some PR_STATIC_ASSERTs laying around:
<http://dxr.mozilla.org/mozilla-central/search?limit=100&redirect=false&q=PR_STATIC_ASSERT%20-path%3Asecurity%2Fnss%2F%20-path%3Ansprpub%2F>.

Also, any C++-language CONFIGURE_STATIC_ASSERTs could be changed to static_asert as well.

Comment 10

[(no longer active)]

(In reply to Justin Lebar [:jlebar] from comment #8)
> Maybe we define MOZ_STATIC_ASSERT only ifndef __cplusplus?

Good idea.

(In reply to Joshua Cranmer [:jcranmer] from comment #9)
> There's also still some PR_STATIC_ASSERTs laying around:
> <http://dxr.mozilla.org/mozilla-central/
> search?limit=100&redirect=false&q=PR_STATIC_ASSERT%20-
> path%3Asecurity%2Fnss%2F%20-path%3Ansprpub%2F>.
> 
> Also, any C++-language CONFIGURE_STATIC_ASSERTs could be changed to
> static_asert as well.

Please file follow-up bugs for these.  Thanks!

Comment 11

[(no longer active)]

(There's more C stuff in libmar, FWIW)

Comment 12

[(no longer active)]

Attachment: static_assert.sh, the driver script

Comment 13

[(no longer active)]

Attachment: Part 1: Autogenerated patch to convert MOZ_STATIC_ASSERT to static_assert in C++ code

Comment 14

[(no longer active)]

Attachment: Part 2: Stop generating MOZ_STATIC_ASSERT in the telemetry code

Comment 15

[(no longer active)]

Attachment: Part 3: Only use MOZ_STATIC_ASSERT in mar.h when we're compiling C code

Comment 16

[(no longer active)]

Attachment: Part 4: Stop #defining MOZ_STATIC_ASSERT in C++ code

Comment 17

[Joshua Cranmer [:jcranmer]]

(In reply to Joshua Cranmer [:jcranmer] from comment #9)
> There's also still some PR_STATIC_ASSERTs laying around:
> <http://dxr.mozilla.org/mozilla-central/
> search?limit=100&redirect=false&q=PR_STATIC_ASSERT%20-
> path%3Asecurity%2Fnss%2F%20-path%3Ansprpub%2F>.

Bug 712936

> Also, any C++-language CONFIGURE_STATIC_ASSERTs could be changed to
> static_asert as well.

Bug 895553

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 777853 [details]
assert_replacer.py, the replacer engine

Looks plausible.  Should be quick enough to skim the script's changes to deal with anything this changes bogusly.

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 777933 [details]
static_assert.sh, the driver script

I suppose those are the only C .h files we have?  And this was verified just by building?  Fine by me, someone'll complain right quick if we mess it up.  :-)

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 777936 [details] [diff] [review]
Part 1: Autogenerated patch to convert MOZ_STATIC_ASSERT to static_assert in C++ code

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

We should do a post-pass afterward searching for

  static_assert\(\([A-Z]

and

  static_assert\(\(mozilla::

to get rid of the parenthesization of type trait queries that contained embedded commas -- needed so the commas wouldn't be trigger macro-parameter parsing, but no longer needed with built-into-the-language static_assert.

::: ipc/chromium/src/base/pickle.cc
@@ +20,1 @@
>  		  "Insufficient alignment");

Your rewrite script didn't unfutz alignment here for whatever reason.

@@ +65,1 @@
>  		      "Pointers have different alignments");

Or here.

@@ +85,1 @@
>  		      "Pointers have different alignments");

Or here.

Comment 21

[Mike Hommey [:glandium]]

Note we can probably build mar as C++

Comment 22

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> Comment on attachment 777933 [details]
> static_assert.sh, the driver script
> 
> I suppose those are the only C .h files we have?

Looks like it.

> And this was verified just by building?

Correct.

>  Fine by me, someone'll complain right quick if we mess it up. 
> :-)

I'm sure they will.  :-)

Comment 23

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Comment on attachment 777936 [details] [diff] [review]
> Part 1: Autogenerated patch to convert MOZ_STATIC_ASSERT to static_assert in
> C++ code
> 
> Review of attachment 777936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should do a post-pass afterward searching for
> 
>   static_assert\(\([A-Z]
> 
> and
> 
>   static_assert\(\(mozilla::
> 
> to get rid of the parenthesization of type trait queries that contained
> embedded commas -- needed so the commas wouldn't be trigger macro-parameter
> parsing, but no longer needed with built-into-the-language static_assert.

OK, I'll do this manually.

> ::: ipc/chromium/src/base/pickle.cc
> @@ +20,1 @@
> >  		  "Insufficient alignment");
> 
> Your rewrite script didn't unfutz alignment here for whatever reason.
> 
> @@ +65,1 @@
> >  		      "Pointers have different alignments");
> 
> Or here.
> 
> @@ +85,1 @@
> >  		      "Pointers have different alignments");
> 
> Or here.

Well, my script tried to handle sane code.  And these locations use tabs.  ;-)

Comment 24

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #21)
> Note we can probably build mar as C++

Bug 895860.

Comment 25

[(no longer active)]

Sadface: <https://tbpl.mozilla.org/?tree=Try&rev=d4b57b18a21c>

Looks like on Android builds, we build the host tools using the system c++ (don't know which gcc that is) which doesn't support C++11.  glandium, is this known?

<https://tbpl.mozilla.org/php/getParsedLog.php?id=25487631&tree=Try>

Comment 26

[Mike Hommey [:glandium]]

Ah yeah, our support for host c++ on cross-compiles kind of sucks. Please file a bug about the host C++ not honoring C++11 and I'll look at it over the week-end.

Comment 27

[(no longer active)]

Attachment: Part 2.1: Stop generating MOZ_STATIC_ASSERT in Web IDL bindings

It escapes me how on earth this did not break my local builds!

Comment 28

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=c62f67601d54

Comment 29

[(no longer active)]

Now the only problem left is bug 895915.

Comment 30

[(no longer active)]

(Note to self: land this with a change to CLOBBER.)

Comment 31

[Jeff Walden [:Waldo]]

Comment on attachment 778462 [details] [diff] [review]
Part 2.1: Stop generating MOZ_STATIC_ASSERT in Web IDL bindings

You could manually correct that static_assert((I while you're here.

Comment 32

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/552bca1bc885
https://hg.mozilla.org/integration/mozilla-inbound/rev/81eb7ee863cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca5f1740017
https://hg.mozilla.org/integration/mozilla-inbound/rev/851bab4e2e3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5506e604ae4

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/552bca1bc885
https://hg.mozilla.org/mozilla-central/rev/81eb7ee863cd
https://hg.mozilla.org/mozilla-central/rev/9ca5f1740017
https://hg.mozilla.org/mozilla-central/rev/851bab4e2e3a
https://hg.mozilla.org/mozilla-central/rev/b5506e604ae4

Comment 34

[Nicholas Nethercote [inactive]]

I filed bug 900275 for removing JS_STATIC_ASSERT.