Closed Bug 895322 Opened 6 years ago Closed 6 years ago

Replace MOZ_STATIC_ASSERT with static_assert

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: emk, Assigned: ehsan)

References

Details

Attachments

(7 files, 3 obsolete files)

After bug 894242, static_assert should be available everywhere.
Attached file static_assert.sh, the driver script (obsolete) —
Attachment #777840 - Flags: review?(jwalden+bmo)
This script helps preserve indentation across multiple lines.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #777842 - Flags: review?(jwalden+bmo)
Comment on attachment 777842 [details]
assert_replacer.py, the replacer engine

Hmm, this script has a bug.
Attachment #777842 - Attachment is obsolete: true
Attachment #777842 - Flags: review?(jwalden+bmo)
Attached file static_assert.sh, the driver script (obsolete) —
Attachment #777840 - Attachment is obsolete: true
Attachment #777840 - Flags: review?(jwalden+bmo)
Attachment #777852 - Flags: review?(jwalden+bmo)
Attachment #777853 - Flags: review?(jwalden+bmo)
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?
Hmm, there is another C usage in media/webrtc (the static asserts in media/webrtc/signaling/src/sipcc/core/includes/ccapi.h)
Maybe we define MOZ_STATIC_ASSERT only ifndef __cplusplus?
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.
(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!
(There's more C stuff in libmar, FWIW)
Attachment #777852 - Attachment is obsolete: true
Attachment #777852 - Flags: review?(jwalden+bmo)
Attachment #777933 - Flags: review?(jwalden+bmo)
Attachment #777961 - Flags: review? → review?(jwalden+bmo)
Attachment #777967 - Flags: review?(jwalden+bmo)
(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
See Also: → 712936
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.
Attachment #777853 - Flags: review?(jwalden+bmo) → review+
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.  :-)
Attachment #777933 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #777961 - Flags: review?(jwalden+bmo) → review+
Attachment #777962 - Flags: review?(jwalden+bmo) → review+
Attachment #777967 - Flags: review?(jwalden+bmo) → review+
Note we can probably build mar as C++
(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.  :-)
(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.  ;-)
(In reply to Mike Hommey [:glandium] from comment #21)
> Note we can probably build mar as C++

Bug 895860.
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>
Flags: needinfo?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
It escapes me how on earth this did not break my local builds!
Attachment #778462 - Flags: review?(jwalden+bmo)
Depends on: 895915
Now the only problem left is bug 895915.
(Note to self: land this with a change to CLOBBER.)
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.
Attachment #778462 - Flags: review?(jwalden+bmo) → review+
I filed bug 900275 for removing JS_STATIC_ASSERT.
You need to log in before you can comment on or make changes to this bug.