Closed
Bug 712936
Opened 12 years ago
Closed 8 years ago
Convert users of PR_STATIC_ASSERT to C++11 static_assert()
Categories
(Core :: General, defect)
Core
General
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)
19.21 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
1.24 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
If you're still looking for someone to do this bug, I would be happy to take this bug.
Updated•12 years ago
|
Assignee: nobody → hung.raymond
Reporter | ||
Comment 2•12 years ago
|
||
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. :-)
Comment 3•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
An eMail from Raymond confirms he's no longer available ... I'm re-assigning this to myself.
Assignee: hung.raymond → markcapella
Status: NEW → ASSIGNED
Comment 5•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
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.
Reporter | ||
Comment 7•12 years ago
|
||
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)
Comment 8•12 years ago
|
||
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
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
Reporter | ||
Comment 11•12 years ago
|
||
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+
Reporter | ||
Comment 12•12 years ago
|
||
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]
Comment 14•12 years ago
|
||
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
Updated•12 years ago
|
Blocks: dieprtypesdie
Version: unspecified → Trunk
Comment 16•11 years ago
|
||
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.
Comment 18•11 years ago
|
||
Venkatesan, could you confirm that you're still working on this?
Flags: needinfo?(vvs)
Comment 19•11 years ago
|
||
Sorry, something else came up. I am unable to continue this further.
Flags: needinfo?(vvs)
Updated•11 years ago
|
Assignee: vvs → nobody
Comment 20•11 years ago
|
||
I'd like to pick up this bug and continue working on it. Is that ok?
Reporter | ||
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
Attachment #763637 -
Flags: review?
Comment 23•11 years ago
|
||
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)
Comment 24•11 years ago
|
||
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)
Comment 25•11 years ago
|
||
Sorry, I messed up generating the patch, will upload a corrected diff when I can.
Comment 26•11 years ago
|
||
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)
Reporter | ||
Comment 27•11 years ago
|
||
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)
Comment 29•11 years ago
|
||
In light of bug 895322, this should really be moving from PR_STATIC_ASSERT to static_assert.
Reporter | ||
Comment 30•11 years ago
|
||
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()
Reporter | ||
Comment 31•11 years ago
|
||
...and if that bug isn't done before patches here happen, I'm happy to make the translation before landing any patches. :-)
Comment 32•11 years ago
|
||
Speaking about this, who wants to file a bug to get rid of MOZ_STATIC_ASSERT_IF? :-)
Comment 33•11 years ago
|
||
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)
Comment 34•11 years ago
|
||
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?
Updated•11 years ago
|
Assignee: leonjcheung → nobody
Comment 35•11 years ago
|
||
(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)
Comment 36•11 years ago
|
||
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
Comment 37•11 years ago
|
||
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)
Comment 38•11 years ago
|
||
Please try not to cancel unrelated requests for information, Anup.
Flags: needinfo?(jwalden+bmo)
Comment 39•11 years ago
|
||
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.
Comment 40•11 years ago
|
||
(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)
Reporter | ||
Comment 41•11 years ago
|
||
...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]
Updated•8 years ago
|
Mentor: Ms2ger
Status: ASSIGNED → NEW
Comment 43•8 years ago
|
||
Yes. https://dxr.mozilla.org/mozilla-central/search?q=PR_STATIC_ASSERT&redirect=false
Assignee | ||
Comment 45•8 years ago
|
||
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)
Assignee | ||
Comment 46•8 years ago
|
||
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 47•8 years ago
|
||
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)
Assignee | ||
Comment 48•8 years ago
|
||
Updated the patch with suggested changes.
Attachment #8783747 -
Attachment is obsolete: true
Attachment #8783994 -
Flags: review?(Ms2ger)
Comment 49•8 years ago
|
||
Comment on attachment 8783994 [details] [diff] [review] long_v3.diff Review of attachment 8783994 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8783994 -
Flags: review?(Ms2ger) → review+
Comment 50•8 years ago
|
||
I think this will be fine, but if y'all would like a try run, let me know.
Keywords: checkin-needed
Assignee | ||
Comment 51•8 years ago
|
||
Are there more instances of P_S_A? After this patch?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(Ms2ger)
Comment 52•8 years ago
|
||
Will check once this patch lands.
Updated•8 years ago
|
Attachment #611259 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #611260 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #765146 -
Attachment is obsolete: true
Comment 53•8 years ago
|
||
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
Comment 54•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/55ebbaf43802
Comment 55•8 years ago
|
||
netwerk/base/nsSocketTransportService2.cpp might still be worth fixing, but apart from that, I think this is fixed.
Flags: needinfo?(Ms2ger)
Assignee | ||
Comment 56•8 years ago
|
||
Attachment #8785345 -
Flags: review?(Ms2ger)
Comment 57•8 years ago
|
||
Comment on attachment 8785345 [details] [diff] [review] v1.diff Review of attachment 8785345 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8785345 -
Flags: review?(Ms2ger) → review+
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Whiteboard: [good first bug][lang=c++][Leave open after merge] → [good first bug][lang=c++]
Comment 58•8 years ago
|
||
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
Comment 59•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d5e83f29513
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 60•8 years ago
|
||
\o/ Thanks for your work!
You need to log in
before you can comment on or make changes to this bug.
Description
•