Closed Bug 712936 Opened 8 years ago Closed 3 years ago

Convert users of PR_STATIC_ASSERT to C++11 static_assert()

Categories

(Core :: General, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Waldo, Assigned: jj, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(2 files, 9 obsolete files)

Now that MOZ_STATIC_ASSERT exists, we should convert everyone[0] using PR_STATIC_ASSERT over to it.  Getting everyone on the same page is a nice reduction in complexity, and it increases the approachability of our code base to not have many ways to do the same thing.  Also, some places that conditionally assert something -- i.e. they're PR_STATIC_ASSERT(!foo || ...) -- could use MOZ_STATIC_ASSERT_IF(foo, ..., "reason") for greater readability.

This conversion isn't just search-and-replace.  P_S_A takes only a compile-time condition as an argument.  M_S_A works like C++11's static_assert and takes two arguments: a condition and a string literal explaining the assertion.  (Compilers that implement the C++11 bit will display the reason if the assertion fails when compiling.)  Existing uses thus need reasons associated with them, and that probably requires getting a little familiar with the relevant code to figure out those reasons.  Also, converted files will need to #include "mozilla/Assertions.h" to get the macro -- not easy to search-and-replace into place.

I'm willing to help anyone interested through this, especially as regards assertions where the reasons behind them aren't obvious.

[0] Code in nsprpub/ and security/nss/ should keep using PR_STATIC_ASSERT, because those directories contain imported code from separate projects, alas.
If you're still looking for someone to do this bug, I would be happy to take this bug.
Assignee: nobody → hung.raymond
Excellent!  Feel free to do this in bite-sized chunks -- incremental improvements that happen faster are generally better than mega improvements that happen slower, or even get bogged down in things and sometimes don't even happen at all.

Also, you can catch me on irc.mozilla.org in #introduction as Waldo, if you have questions -- and if I'm not around to answer a question, probably someone else will be who can as well.  So just ask in the channel, and you should get answers.  :-)
Depends on: 729142
Attached patch Patch XPCOM Files subset (v1) (obsolete) — Splinter Review
I dropped an email to Raymond Hung to see if he is still pursuing this bug, and offering to finish it for him, as it's been a few months since he was assigned.

Of the 64 files I found affected by the request, this attached patch addresses the subset in the \XPCOM directory.

I Raymond wishes, he could use this as a starting point.
Attachment #609094 - Flags: feedback?(jwalden+bmo)
An eMail from Raymond confirms he's no longer available ... I'm re-assigning this to myself.
Assignee: hung.raymond → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v2) (obsolete) — Splinter Review
New version of the patch ... includes files in the XPCOM, SECURITY, TOOLKIT, and LAYOUT directories ... 28 of the total 64 to be addressed. Two things I note:

1) I've only actually had to include file "mozilla/Assertions.h" in the xpcom/build/nsXULAppAPI.h file to build successfully ...

2) I've skipped changes to three of the 5 SECURITY files ... specifically those in the /security/nss/lib/ directory, as I can't seem to make them locate the assertions.h file ... it may be a make/build configuration issue ... I'm researching ...
Attachment #609094 - Attachment is obsolete: true
Attachment #609209 - Flags: feedback?(jwalden+bmo)
Attachment #609094 - Flags: feedback?(jwalden+bmo)
Oh cool ... just spotted this above
[0] Code in nsprpub/ and security/nss/ should keep using PR_STATIC_ASSERT, because those directories contain imported code from separate projects, alas.
Comment on attachment 609209 [details] [diff] [review]
Patch (v2)

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

First, a general comment.  These messages generally should try to explain to the reader 1) why the assertion failed, and 2) ideally how to fix it.  A bunch of the reasons you've chosen simply repeat the condition being tested, which is not particularly helpful.  If we weren't trying to emulate the interface of C++11 and C11's static_assert(cond, "reason") functionality, we'd probably make the reason optional.  But as they don't make it optional, we don't, so we have to figure out something vaguely sane here.  :-\  This may require case-by-case consideration, I think.

It looks like you've changed a few uses in NSS.  NSS is a separate project, and it can't depend on mfbt or its macros (at least not yet, who knows what the future could bring).  So you should revert those changes entirely.  In theory NSS would be open to commenting these assertions with explanations, but changing NSS seems more trouble than it's worth, honestly.  :-\

Looking at how this is playing out, I think I might perhaps have been optimistic in thinking this was always good-first-bug material.  :-\  Figuring out what to say isn't always obvious given the subtle invariants in play.  And saying it right -- without simply repeating oneself -- can be harder still.  I reviewed these files, and I think these changes (with whatever nitpicks I listed) work:

FramePropertyTable.cpp
nsPresContext.cpp
inDOMUtils.cpp
nsNSSComponent.cpp
Telemetry.cpp
nsAppRunner.cpp
MapsMemoryReporter.cpp
nsXULAppAPI.h
nsSubstring.cpp

The changes to the other files, however, I don't quite understand well enough to review, don't like the wording essentially repeating the assertion itself (without providing any deeper explanation, or help in deciding how to resolve a failure of the assertion), or something else.  Mind picking out the changes to just the above files into a patch, and leaving the rest of the changes to a second patch for deeper examination, possibly by people who aren't me?

::: layout/base/FramePropertyTable.cpp
@@ +70,5 @@
>      // We need to expand the single current entry to an array
>      PropertyValue current = entry->mProp;
>      entry->mProp.mProperty = nsnull;
> +    MOZ_STATIC_ASSERT(sizeof(nsTArray<PropertyValue>) <= sizeof(void *),
> +                      "Property array entry size must not be larger than (void *)");

How about "Property array must fit entirely within entry->mProp.mValue" instead?  The assertion's checking that the array allocated below can be allocated in the memory for mValue, without overflowing it, so this message is clearer.  (The assertion would be much clearer if it used |sizeof(entry->mProp.mValue)|, but as hg annotations indicate, that apparently broken the Sun compiler.  :-(  Alas...)

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1657,5 @@
> +  MOZ_STATIC_ASSERT(nsINSSErrorsService::NSS_SEC_ERROR_BASE == SEC_ERROR_BASE &&
> +                    nsINSSErrorsService::NSS_SEC_ERROR_LIMIT == SEC_ERROR_LIMIT &&
> +                    nsINSSErrorsService::NSS_SSL_ERROR_BASE == SSL_ERROR_BASE &&
> +                    nsINSSErrorsService::NSS_SSL_ERROR_LIMIT == SSL_ERROR_LIMIT,
> +                    "You must update the values in nsINSSErrorsService.idl");

You can get rid of the comment above this, now that the assertion message says the same thing.

(This file isn't part of NSS, so it's fine to change it.  Just to be clear...)

::: security/nss/cmd/multinit/multinit.c
@@ +159,5 @@
> +    MOZ_STATIC_ASSERT(sizeof(options_init)/sizeof(secuCommandFlag) == opt_last,
> +                      "Our Help table is not up to date");
> +    MOZ_STATIC_ASSERT(sizeof(options_init)/sizeof(secuCommandFlag) == 
> +		                  sizeof(options_des)/sizeof(commandDescript),
> +                      "Our Help table is not up to date");

Don't change these, even if changing them does compile.  (I'm guessing this file isn't built, or something, which is why your change here managed to sneak through things.)

::: xpcom/build/nsXULAppAPI.h
@@ +365,5 @@
>    "ipdlunittest"
>  };
>  
> +MOZ_STATIC_ASSERT(sizeof(kGeckoProcessTypeString) /
> +                  sizeof(kGeckoProcessTypeString[0]) ==

So, this is checking that the length of the kGeckoProcessTypeString array is GeckoProcessType_End.  There's a better way to calculate that length, usually -- mozilla::ArrayLength.  Unfortunately in the C++ that many compilers implement now, it's not possible to use that in a place (like a static assertion) that expects a constant expression.  So for now, use NS_ARRAY_LENGTH(kGeckoProcessTypeString) instead.

@@ +367,5 @@
>  
> +MOZ_STATIC_ASSERT(sizeof(kGeckoProcessTypeString) /
> +                  sizeof(kGeckoProcessTypeString[0]) ==
> +                  GeckoProcessType_End,
> +                  "Invalid Gecko Process Type String");

I'd say "Array length mismatch".

::: xpcom/ds/nsExpirationTracker.h
@@ +113,5 @@
>      nsExpirationTracker(PRUint32 aTimerPeriod)
>        : mTimerPeriod(aTimerPeriod), mNewestGeneration(0),
>          mInAgeOneGeneration(false) {
> +      MOZ_STATIC_ASSERT(K >= 2 && K <= nsExpirationState::NOT_TRACKED,
> +                        "This object cannot use a timer for tracking");

See the comment on K in the file:

 * The parameter K is the number of generations that will be used. Increasing
 * the number of generations narrows the window within which we promise
 * to fire notifications, at a slight increase in space cost for the tracker.
 * We require 2 <= K <= nsExpirationState::NOT_TRACKED (currently 15).

This is what we want to state here, albeit more briefly -- your message isn't informative in quite the same way.  I suggest "Unsupported number of generations (must be 2 <= K <= 15)" instead.

::: xpcom/glue/nsStringAPI.h
@@ +1118,5 @@
>    typedef NS_ConvertASCIItoUTF16 nsLiteralString;
>  #endif
>  
>  /* Check that PRUnichar is unsigned */
> +MOZ_STATIC_ASSERT(PRUnichar(-1) > PRUnichar(0), "PRUnichar must be unsigned");

I'd say "PRUnichar is by definition an unsigned type".
Attachment #609209 - Flags: feedback?(jwalden+bmo)
Re: we'd probably make the reason optional

True enough. And as there are no explanations as of the start of this change, We could have simply modified every one of the items to say MOZ_STATIC_ASSERT (foocondition, "Hey ! The build is crashing, check around here and fix stuff") and any decent coder should have had enough to go on.

Re; This may require case-by-case consideration

Also, true. Note that this first pass was meant to be a baseline. Not being familiar with the code, the thought was to get "something" in place, and then depend on the fact that there would be ample / multiple opportunities for stakeholders, module owners, and mentors to provide feedback, which of course would be acted on :)

Re: It looks like you've changed a few uses in NSS

Yah, I didn't notice the final requirement of the original request until after I posted the patch. This will be backed out / avoided.

Re: Mind picking out the changes to just the above files into a patch, and leaving the rest of the changes to a second patch for deeper examination, possibly by people who aren't me?

Not at all. We can start with a patch you're comfortable / familiar with, get that in place, then defer the rest for as long as time permits for review and feedback by others with better knowledge / strong opinions.

I'll have some time in the next several days and will provide a revised patch for you to review. Thanks for your attention -- mark :p
Attached patch Patch (v3) (obsolete) — Splinter Review
Ok, this patch is for the (9) files you asked to me to limit it to, and two that were in your nits list for a total of (11).

Note that for nsXULAppAPI.h the modification for the array item count that you asked for caused a strange build error in my WIN 7/VC2010 Express environment involving the the XRE_API further down.

MOZ_STATIC_ASSERT(NS_ARRAY_LENGTH(kGeckoProcessTypeString) ==
                  GeckoProcessType_End,
                  "Array length mismatch");

Causes problems with:

XRE_API(nsresult,
        XRE_InitChildProcess, (int aArgc,
                               char* aArgv[],
                               GeckoProcessType aProcess))

I've attached a screenshot for reference, and have left the code as is, so this patch builds successfully, until an answer is found.
Attachment #609209 - Attachment is obsolete: true
Attachment #611259 - Flags: feedback?(jwalden+bmo)
Attached file Build Error Messages (obsolete) —
Comment on attachment 611259 [details] [diff] [review]
Patch (v3)

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b66d04ae6ae0

Good stuff.  There's one substantive-ish comment below that I addressed myself when checking in.  I also noticed in doing a last once-over before pushing that one of the lines you changed in the patch had trailing whitespace, so I removed that.  Otherwise, this looks great -- thanks!

::: xpcom/build/nsXULAppAPI.h
@@ +370,5 @@
> +                  GeckoProcessType_End,
> +                  "Array length mismatch");
> +//MOZ_STATIC_ASSERT(NS_ARRAY_LENGTH(kGeckoProcessTypeString) ==
> +//                  GeckoProcessType_End,
> +//                  "Array length mismatch");

(In reply to Mark Capella [:capella] from comment #9)
> Note that for nsXULAppAPI.h the modification for the array item count that
> you asked for caused a strange build error in my WIN 7/VC2010 Express
> environment involving the the XRE_API further down.

Nicely done!  Looks like a compiler bug of some sort.  Let's add a comment saying NS_ARRAY_LENGTH causes MSVC10 internal compiler errors but otherwise get rid of the commented-out version.  Someone doing code archaeology in the distant future can fix it then.
Attachment #611259 - Flags: feedback?(jwalden+bmo) → review+
There's more patchwork to come, so inbound mergers shouldn't close this yet.  :-)
Whiteboard: [good first bug][mentor=Waldo][lang=c++] → [good first bug][mentor=Waldo][lang=c++][Leave open after merge]
Waldo, this still shows on my list of stufftodo since I'm assigned the bug from having done the original piece ... 

The patch is still open from comment#2 "leaving the rest of the changes to a second patch for deeper examination, possibly by people who aren't me?" I'm going to drop the assigned person back to nobody@mozilla for when you're to do that.
Assignee: markcapella → nobody
Duplicate of this bug: 797238
Version: unspecified → Trunk
Is it fine for me to take up this bug and work on it? I thought I could start contributing to the codebase through this bug.
Of course!
Assignee: nobody → vvs
Venkatesan, could you confirm that you're still working on this?
Flags: needinfo?(vvs)
Sorry, something else came up. I am unable to continue this further.
Flags: needinfo?(vvs)
Assignee: vvs → nobody
I'd like to pick up this bug and continue working on it. Is that ok?
Sure!  Feel free to ask here or on IRC if you have any questions -- I'm happy to answer, or asking in #introduction will probably get you pointed in the right direction.
Assignee: nobody → leonjcheung
Comment on attachment 763637 [details] [diff] [review]
Patch for MOZ_STATIC_ASSERT in several files.

Note that untargeted review requests (review?) tend to fall through the cracks. Waldo's a good target in this case.
Attachment #763637 - Flags: review? → review?(jwalden+bmo)
In addition to my previous attachment, this patch about finishes converting all instances of PR_STATIC_ASSERT that I could find.
Attachment #765124 - Flags: review?(jwalden+bmo)
Sorry, I messed up generating the patch, will upload a corrected diff when I can.
Fixed the patch diff and combined both progress patches into one.
Attachment #763637 - Attachment is obsolete: true
Attachment #765124 - Attachment is obsolete: true
Attachment #763637 - Flags: review?(jwalden+bmo)
Attachment #765124 - Flags: review?(jwalden+bmo)
Attachment #765146 - Flags: review?(jwalden+bmo)
Comment on attachment 765146 [details] [diff] [review]
Complete patch containing all changes

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

I started reviewing this, but I didn't get very far in before realizing that I'm probably not the right person to look at this.  Essentially what's involved is elaborating the relevant requirements, that each static assertion is verifying.  Sometimes it's obvious why something is as it is.  Often, it's not.  Thus it's probably best for reviewers of the relevant bits of code in question (which is generally easily figured out through |hg log| on the relevant files) to be looking over these changes -- a few files at a time, grouped by code topic (usually determined by path within the tree).  I kind of hate to drop this :-( but there's really no way for me to timely review all these, if I'm to review them effectively.  :-\

The files I looked at were only these:

content/base/public/nsINode.h
content/base/src/nsDocument.cpp
content/events/src/nsDOMDataTransfer.cpp
gfx/src/nsRect.cpp

Whatever comments I made on them elaborate the state of them, in terms of what I think needs changing, or doesn't need changing, to each of them.  (Watch out for the couple patch-wide comments interspersed in there, tho!)

::: content/base/public/nsINode.h
@@ +173,5 @@
>    NODE_TYPE_SPECIFIC_BITS_OFFSET =        22
>  };
>  
>  // Make sure we have space for our bits
> +#define ASSERT_NODE_FLAGS_SPACE(n) MOZ_STATIC_ASSERT(WRAPPER_CACHE_FLAGS_BITS_USED + (n) <= 32, "Not enough space in the node flags space.")

This is really long for a single line.  Let's add in some linebreaks:

#define ASSERT_NODE_FLAGS_SPACE(n) \
  MOZ_STATIC_ASSERT(WRAPPER_CACHE_FLAGS_BITS_USED + (n) <= 32, \
                    "Not enough space in the node flags space")

Also, nitpicky, but let's not have trailing periods in these messages, on the assumption that the compiler-chosen context might not expect it.

@@ +1326,5 @@
>    };
>  
>    void SetBoolFlag(BooleanFlag name, bool value) {
> +    MOZ_STATIC_ASSERT(BooleanFlagCount <= 8*sizeof(mBoolFlags), 
> +                      "Too many BooleanFlags to pack into mBoolFlags"); 

This looks like a general nit that needs to be fixed a bunch of places -- you should get rid of trailing whitespace.  It sticks out like a sore thumb in many editors, and the Splinter review display highlights it pretty clearly.

::: content/base/src/nsDocument.cpp
@@ +10,5 @@
>  
>  #include "mozilla/DebugOnly.h"
>  #include "mozilla/Util.h"
>  #include "mozilla/Likely.h"
> +#include "mozilla/Assertions.h"

Let's alphabetize these while we're here.

::: content/events/src/nsDOMDataTransfer.cpp
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/Util.h"
> +#include "mozilla/Assertions.h"

Alphabetize.

@@ +167,5 @@
>    }
>  
> +  MOZ_STATIC_ASSERT(nsIDragService::DRAGDROP_ACTION_NONE == 0, 
> +                    "nsIDragService::DRAGDROP_ACTION_NONE not assigned "
> +                    "to correct value. Check source/widget/nsIDragService.idl.");

It looks like for these, it's not so much that they're not assigned the correct values.  It's that nsDOMDataTransfer::sEffects has to be synchronized with that ordering.  So a better reason would be:

  MOZ_STATIC_ASSERT(nsIDragService::DRAGDROP_ACTION_NONE == 0,
                    "enum value out of sync with nsDOMDataTransfer::sEffects");

And the similar change for all the rest of them.

Note that most compilers will include the failing assertion expression in the compiler output, so it's not necessary to identify the specific thing that went wrong.  (Granted, we don't compile as C++11 everywhere, so we won't always get compiler-generated error output.  But this is a slowly dying situation, and in any case if someone breaks a static assertion they're going to have to look at the expression one way or another anyway.)
Attachment #765146 - Flags: review?(jwalden+bmo)
Josh, could you find someone to look at this?
Flags: needinfo?(josh)
In light of bug 895322, this should really be moving from PR_STATIC_ASSERT to static_assert.
See Also: → 895322
Yup.  Easy change to make, certainly.
Summary: Convert users of PR_STATIC_ASSERT to MOZ_STATIC_ASSERT or MOZ_STATIC_ASSERT_IF → Convert users of PR_STATIC_ASSERT to C++11 static_assert()
...and if that bug isn't done before patches here happen, I'm happy to make the translation before landing any patches.  :-)
Speaking about this, who wants to file a bug to get rid of MOZ_STATIC_ASSERT_IF?  :-)
I'm not clear on what the status of the work here is. Could somebody enlighten me so I can properly represent it to potential candidates to complete it?
Flags: needinfo?(josh)
Sorry, I'm unable to work on this anymore. Jeff Walden last requested that I contact various contributors that are most familiar with the source files modified by my patch in order to verify that my error messages are as descriptive as possible. Perhaps someone else can take care of this?
Assignee: leonjcheung → nobody
(In reply to Josh Matthews [:jdm] from comment #33)
> I'm not clear on what the status of the work here is. Could somebody
> enlighten me so I can properly represent it to potential candidates to
> complete it?

-> Waldo.
Flags: needinfo?(jwalden+bmo)
Hi, I am interested in working on this bug. So, I request you to assigm it to me and guide me on how ti get started with because I am a newbie.

Thanks in advance,
A.Anup
Hi, I am interested in working on this bug. So, I request you to assigm it to me and guide me on how ti get started with because I am a newbie.

Thanks in advance,
A.Anup
Flags: needinfo?(jwalden+bmo)
Please try not to cancel unrelated requests for information, Anup.
Flags: needinfo?(jwalden+bmo)
I think I duplicated part of your work here in bug 903012. Feel free to comment there if you don't like my error messages.
(In reply to Josh Matthews [:jdm] (unavailable until 8/18) from comment #33)
> I'm not clear on what the status of the work here is. Could somebody
> enlighten me so I can properly represent it to potential candidates to
> complete it?

We appear to have a patch that moves users of PR_STATIC_ASSERT to MOZ_STATIC_ASSERT. As we removed MOZ_STATIC_ASSERT, the patch should be modified to instead use static_assert; also, added includes that are now unnecessary should be removed.

The main part of this bug was figuring out why the assertions were asserting what they did. This has already happened in the attached patch, so the remaining work is mostly search-and-replace, addressing the review nits from comment 27, and driving the patch through review by finding people who know the changed code to review the correctness of the new messages.
Flags: needinfo?(jwalden+bmo)
...yeah, much as I'd like to help out here, I probably shouldn't be on the hook for it, given my plate now.  :-(  Asking brief questions may still get answers, tho, so don't hesitate to ask.
Whiteboard: [good first bug][mentor=Waldo][lang=c++][Leave open after merge] → [good first bug][lang=c++][Leave open after merge]
Mentor: Ms2ger
Status: ASSIGNED → NEW
Is this bug still in existence?
Flags: needinfo?(Ms2ger)
Indeed it is. Let me know if you'd like some help.
Flags: needinfo?(Ms2ger)
Attached patch long_v1.diff (obsolete) — Splinter Review
I am trying to approach this problem in an incremental update manner. So in the meantime all things in this patch get approved I would work on other parts too.
Attachment #8783656 - Flags: review?(Ms2ger)
Attached patch long_v2.diff (obsolete) — Splinter Review
Kind of removed all the instances of P_S_A with C++11 static_assert.
Assignee: nobody → jinank94
Attachment #8783656 - Attachment is obsolete: true
Attachment #8783656 - Flags: review?(Ms2ger)
Attachment #8783747 - Flags: review?(Ms2ger)
Comment on attachment 8783747 [details] [diff] [review]
long_v2.diff

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

This is great, thank you! I've got a few minor comments below; can you please upload a new patch with those comments addressed, and ask me to review again.

::: gfx/thebes/gfxFontEntry.cpp
@@ +851,3 @@
>  #define FEATURE_SCRIPT_MASK 0x000000ff // script index replaces low byte of tag
>  
>  // check for too many script codes

This comment can be removed now.

::: layout/base/nsDisplayList.cpp
@@ +765,4 @@
>    mFrameToAnimatedGeometryRootMap.Put(aReferenceFrame, &mRootAGR);
>  
>    nsCSSRendering::BeginFrameTreesLocked();
> +  static_assert(nsDisplayItem::TYPE_MAX < (1 << nsDisplayItem::TYPE_BITS), 

Remove the space at the end of this line

::: layout/generic/nsFrameSetFrame.cpp
@@ +237,3 @@
>    ourContent->GetColSpec(&mNumCols, &colSpecs);
>  
>    // Maximum value of mNumRows and mNumCols is NS_MAX_FRAMESET_SPEC_COUNT

This comment can be removed now.

@@ +242,4 @@
>    mRowSizes  = MakeUnique<nscoord[]>(mNumRows);
>    mColSizes  = MakeUnique<nscoord[]>(mNumCols);
>  
>    // Ensure we can't overflow numCells

This comment can be removed now.

@@ +428,3 @@
>                                            nscoord*              aValues)
>  {
>    // aNumSpecs maximum value is NS_MAX_FRAMESET_SPEC_COUNT

This comment can be removed now.

::: layout/xul/nsXULPopupManager.h
@@ +112,4 @@
>  #define NS_DIRECTION_IS_BLOCK_TO_EDGE(dir) (dir == eNavigationDirection_First ||    \
>                                              dir == eNavigationDirection_Last)
>  
> +static_assert(NS_STYLE_DIRECTION_LTR == 0 && NS_STYLE_DIRECTION_RTL == 1, 

Space here too.

::: netwerk/dns/nsHostResolver.cpp
@@ +64,4 @@
>  #define LongIdleTimeoutSeconds  300           // for threads 1 -> HighThreadThreshold
>  #define ShortIdleTimeoutSeconds 60            // for threads HighThreadThreshold+1 -> MAX_RESOLVER_THREADS
>  
> +static_assert(HighThreadThreshold <= MAX_RESOLVER_THREADS, 

And here.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +596,4 @@
>      // rand() provides different amounts of PRNG on different platforms.
>      // 15 or 31 bits are common amounts.
>  
> +    static_assert(RAND_MAX >= 0xfff, "RAND_MAX should be >= 15 bits");

Is this not 12 bits?

::: parser/html/nsHtml5NamedCharacters.cpp
@@ +100,3 @@
>  };
>  
>  /* check that the start positions will fit in 16 bits */

This comment can be removed now.

::: toolkit/xre/nsUpdateDriver.cpp
@@ +214,3 @@
>  GetStatusFileContents(nsIFile *statusFile, char (&buf)[Size])
>  {
>    // The buffer needs to be large enough to hold the known status codes

This comment can be removed now.
Attachment #8783747 - Flags: review?(Ms2ger)
Attached patch long_v3.diffSplinter Review
Updated the patch with suggested changes.
Attachment #8783747 - Attachment is obsolete: true
Attachment #8783994 - Flags: review?(Ms2ger)
Comment on attachment 8783994 [details] [diff] [review]
long_v3.diff

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

Thanks!
Attachment #8783994 - Flags: review?(Ms2ger) → review+
I think this will be fine, but if y'all would like a try run, let me know.
Keywords: checkin-needed
Are there more instances of P_S_A? After this patch?
Flags: needinfo?(Ms2ger)
Will check once this patch lands.
Attachment #611259 - Attachment is obsolete: true
Attachment #611260 - Attachment is obsolete: true
Attachment #765146 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ebbaf43802
Convert users of PR_STATIC_ASSERT to C++11 static_assert(). r=Ms2ger
Keywords: checkin-needed
netwerk/base/nsSocketTransportService2.cpp might still be worth fixing, but apart from that, I think this is fixed.
Flags: needinfo?(Ms2ger)
Attached patch v1.diffSplinter Review
Attachment #8785345 - Flags: review?(Ms2ger)
Comment on attachment 8785345 [details] [diff] [review]
v1.diff

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

Thanks!
Attachment #8785345 - Flags: review?(Ms2ger) → review+
Whiteboard: [good first bug][lang=c++][Leave open after merge] → [good first bug][lang=c++]
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5e83f29513
Convert users of PR_STATIC_ASSERT to C++11 static_assert(). r=Ms2ger
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d5e83f29513
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
\o/ Thanks for your work!
You need to log in before you can comment on or make changes to this bug.