Closed Bug 1127201 Opened 5 years ago Closed 5 years ago

Remove NS_ABORT_IF_FALSE in favour of MOZ_ASSERT

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(5 files, 2 obsolete files)

NS_ABORT_IF_FALSE and MOZ_ASSERT serve the same purpose. We don't need both.

rgrep tells me that NS_ABORT_IF_FALSE is used 1,786 times in mozilla-central and MOZ_ASSERT is used 29,214 times. So let's get rid of NS_ABORT_IF_FALSE.
I plan to land these changes in a single patch, but I've split the changes into
two parts to ease reviewing.

This patch consists purely of a global s/NS_ABORT_IF_FALSE/MOZ_ASSERT/ that I
did with Perl. It merits only skimming.
Attachment #8556300 - Flags: review?(jwalden+bmo)
This part consists of all the manual changes, which merit more careful review.

- It removes NS_ABORT_IF_FALSE.

- It modifies some former NS_ABORT_IF_FALSE calls to account for the main
  difference between NS_ABORT_IF_FALSE and MOZ_ASSERT -- the former allows
  string variables for the 2nd argument, while the latter allows only string
  literals.

  This mostly consisted of removing the printing of an unexpected value, which
  I think is a loss of little concern. In one place (nsPresArena.cpp) I changed
  the MOZ_ASSERT to an fprintf of address data along with a MOZ_CRASH (this is
  valid because it is DEBUG-only code.)
Attachment #8556305 - Flags: review?(jwalden+bmo)
Comment on attachment 8556305 [details] [diff] [review]
(part 1b) - Remove NS_ABORT_IF_FALSE in favour of MOZ_ASSERT

>       MOZ_ASSERT(animDirection.list->mValue.GetUnit() == eCSSUnit_Enumerated,
>-                        nsPrintfCString("Invalid animation-direction unit %d",
>-                                        animDirection.list->mValue.GetUnit()).get());
>+                 "Invalid animation-direction unit %d");

The %d doesn't seem right.

> 
>   default:
>-    MOZ_ASSERT(false,
>-                      nsPrintfCString("unexpected unit %d",
>-                                      aValue.GetUnit()).get());
>+    MOZ_ASSERT(false, "unexpected unit %d");
>   }

same.
(In reply to Nicholas Nethercote [:njn] from comment #2)
> - It modifies some former NS_ABORT_IF_FALSE calls to account for the main
>   difference between NS_ABORT_IF_FALSE and MOZ_ASSERT -- the former allows
>   string variables for the 2nd argument, while the latter allows only string
>   literals.

Why not fix MOZ_ASSERT?  Using nsPrintfCString() is quite useful.
Comment on attachment 8556300 [details] [diff] [review]
(part 1a) - Remove NS_ABORT_IF_FALSE in favour of MOZ_ASSERT

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

You're going to need to do something smarter here that preserves alignment choices, when the macro call spans multiple lines.  Past experience with bug 348748 comment 13 and bug 348748 comment 15 (and back starting with #c9 or so) indicates it is not acceptable to disrupt blame on all these lines once here, then again, piecemeal, when many someones fix up each individual place's broken vertical alignment.

My script in bug 348748 might be a reasonable starting point for doing this, although it has many different peculiarities tying it closely to the problem space it confronted -- always two-argument macro calls (versus either one argument or two), second argument never contains ',' (where a macro string might), second argument might be concatenated string literals spanning multiple lines, and possibly others.

The other possibility is just skim the patch and fix all instances manually.  It might not be that bad, for a one-time thing.  Up to you.
Attachment #8556300 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8556305 [details] [diff] [review]
(part 1b) - Remove NS_ABORT_IF_FALSE in favour of MOZ_ASSERT

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

I don't have a problem with this, exactly, but in light of r- on the first part this r+ is not immediately useful, I guess.

Regarding enhancing MOZ_ASSERT to handle printf-style interpolation, in general it's been a very unusual thing to want to do.  I can think of two, maybe three instances where I've seen someone do something that effectively worked around that lack.  So I'm a bit dubious on the merits regarding introducing that complication here.

However, I don't think we need to confront that question to get 99% of the benefits here.  If removing the printf-like uses here is truly that big a deal, I'd say just leave NS_ABORT_IF_FALSE around with only the few weird places touched here using it, or switch these to fprintf and a subsequent MOZ_ASSERT.  I'm really not worried about new instances of N_A_I_F creeping into the tree in any large quantities if we leave it around, not when there are 29k+ uses of MOZ_ASSERT.

::: layout/base/nsPresArena.cpp
@@ +79,5 @@
> +                          "found %.16llx "
> +                          "errors in bits %.16llx",
> +                          uint64_t(mozPoisonValue()),
> +                          uint64_t(val),
> +                          uint64_t(mozPoisonValue() ^ val));

Nothing against the conversion overall, but.  This is the wrong way to format uint64_t.  llx is not guaranteed to be the specifier for uint64_t, and indeed is not with some compilers.  Use #include "mozilla/IntegerPrintfMacros.h" and PRIx64 instead.
Attachment #8556305 - Flags: review?(jwalden+bmo) → review+
I don't think supporting use of nsPrintfCString, in the existing pattern used with NS_ABORT_IF_FALSE, should be much work; I'd think it would just require not using token-pasting in the macro itself (MOZ_ASSERT_HELPER2), but instead giving MOZ_ReportAssertionFailure a second string parameter so that the string concatenation can be done inside.
ASSERT_UNLESS_FUZZING() (which is defined multiple times!) caused problems due
when __VA_ARGS__ was empty which is most of the time. So I just disallowed the
optional string, which was only used in a small fraction of the occurrences.

I don't particularly like this patch. I'm not convinced its any better than
just removing the nsPrintfCString()s like I did earlier, but I've done it to at
least show what's involved.
Attachment #8558814 - Flags: review?(jwalden+bmo)
I first converted all the multi-line NS_ABORT_IF_FALSE calls. There was lots of
variation in indentation so I did this semi-automatically -- I visited every
callsite in my editor, used macros to convert the common patterns, and did the
less common patterns by hand.
Attachment #8558815 - Flags: review?(jwalden+bmo)
I did this with an automated, global search-and-replace. It needs a few manual
fix-ups to make it compile.
Attachment #8558817 - Flags: review?(jwalden+bmo)
This is fix-ups for the automated patch, and the actual removal of the macro.
It includes removal of some unnecessary #include statements.
Attachment #8558818 - Flags: review?(jwalden+bmo)
Attachment #8556300 - Attachment is obsolete: true
Attachment #8556305 - Attachment is obsolete: true
+1 to removing NS_ABORT_IF_FALSE entirely. I really don't want coders adding NS_ABORT_IF_FALSE thinking when they want MOZ_RELEASE_ASSERT.
Comment on attachment 8558814 [details] [diff] [review]
(part 1) - Let MOZ_ASSERT take a string variable as the second arg

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

I'm kind of dubious about getting rid of the assertion messages on all these other files than Assertions.h.  But hey, your code...

::: mfbt/Assertions.h
@@ +279,5 @@
>   * an attempt is made to invoke any existing debugger, and execution halts.
>   * MOZ_ASSERT is fatal: no recovery is possible.  Do not assert a condition
>   * which can correctly be falsy.
>   *
> + * The optional explanation-string, if provided, must be a |char*| (string

const char*
Attachment #8558814 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8558815 [details] [diff] [review]
(part 2a) - Convert NS_ABORT_IF_FALSE to MOZ_ASSERT (multi-line cases)

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

::: dom/base/NodeInfo.cpp
@@ +104,5 @@
>        SetDOMStringToNull(mLocalName);
>        break;
>      default:
> +      MOZ_ASSERT(aNodeType == UINT16_MAX,
> +                 "Unknown node type");

Reduce this to a single line?  Probably wasn't just because of line length, but you look good now.

::: dom/base/nsDOMWindowUtils.cpp
@@ +2780,5 @@
>      property = eCSSProperty_UNKNOWN;
>    }
>  
> +  MOZ_ASSERT(property == eCSSProperty_UNKNOWN ||
> +             !nsCSSProps::IsShorthand(property),

This might be one-linable as far as the condition goes, too.

::: dom/base/nsGenericDOMDataNode.cpp
@@ +45,5 @@
> +  MOZ_ASSERT(mNodeInfo->NodeType() == nsIDOMNode::TEXT_NODE ||
> +             mNodeInfo->NodeType() == nsIDOMNode::CDATA_SECTION_NODE ||
> +             mNodeInfo->NodeType() == nsIDOMNode::COMMENT_NODE ||
> +             mNodeInfo->NodeType() ==
> +               nsIDOMNode::PROCESSING_INSTRUCTION_NODE ||

This equality has to fit in 80ch, right?

@@ +57,5 @@
> +  MOZ_ASSERT(mNodeInfo->NodeType() == nsIDOMNode::TEXT_NODE ||
> +             mNodeInfo->NodeType() == nsIDOMNode::CDATA_SECTION_NODE ||
> +             mNodeInfo->NodeType() == nsIDOMNode::COMMENT_NODE ||
> +             mNodeInfo->NodeType() ==
> +               nsIDOMNode::PROCESSING_INSTRUCTION_NODE ||

Ditto.

::: dom/svg/SVGPointList.h
@@ +121,5 @@
>    }
>  
>    void ReplaceItem(uint32_t aIndex, const SVGPoint &aPoint) {
> +    MOZ_ASSERT(aIndex < mItems.Length(),
> +               "DOM wrapper caller should have raised INDEX_SIZE_ERR");

Not that it's something to change here, but it seems really strange that we have umpteen implementations of these two methods, with umpteen versions of the same assertion.  Someone should look into whether these could all be done/handled within a single base class somehow.

::: dom/svg/nsSVGAttrTearoffTable.h
@@ +55,5 @@
>    bool found =
>  #endif
>      mTable->Get(aSimple, &tearoff);
> +  MOZ_ASSERT(!found || tearoff,
> +             "NULL pointer stored in attribute tear-off map");

s/NULL/null/, same thing about NULL/nsnull being obsolete

::: gfx/gl/GLContextProviderGLX.cpp
@@ +838,1 @@
>          "glXMakeCurrent failed to release GL context before we call glXDestroyContext!");

Fix the indentation on the message to be like all the other spilled-across-lines indentation, please.

::: image/decoders/nsJPEGDecoder.cpp
@@ +573,5 @@
>      break;
>  
>    case JPEG_ERROR:
> +    MOZ_ASSERT(0, "Should always return immediately after error and"
> +                  " not re-enter decoder");

Refactor to s/0/false/ and put the message on separate line(s)?

::: ipc/glue/Shmem.cpp
@@ +185,5 @@
>              char** aData,
>              char** aBackSentinel)
>  {
> +  MOZ_ASSERT(aSegment && aFrontSentinel && aData && aBackSentinel,
> +             "NULL param(s)");

Maybe s/NULL/null/ now that we don't use NULL (or even nsnull).

::: layout/base/FrameLayerBuilder.cpp
@@ +4370,5 @@
>      NS_ASSERTION(AppUnitsPerDevPixel(cdi->mItem) == aAppUnitsPerDevPixel,
>                   "a painted layer should contain items only at the same zoom");
>  
> +    MOZ_ASSERT(clip.HasClip() ||
> +               clip.GetRoundedRectCount() == 0,

I think you can fit both conditions on one line now.

::: layout/base/nsDocumentViewer.cpp
@@ +3592,5 @@
>        selCon->RepaintSelection(nsISelectionController::SELECTION_NORMAL);
>      }
>    } else {
> +    MOZ_ASSERT(eventType.EqualsLiteral("blur"),
> +               "Unexpected event type");

This should fit on one line now.

::: layout/base/nsPresArena.cpp
@@ +76,5 @@
> +        MOZ_ASSERT(val == mozPoisonValue(),
> +                   nsPrintfCString("PresArena: poison overwritten; "
> +                                   "wanted %.16llx "
> +                                   "found %.16llx "
> +                                   "errors in bits %.16llx",

PRIx64 as I mentioned on the previous patches.

::: layout/generic/nsLineLayout.h
@@ +189,5 @@
>      // provided to the line layout. However, floats should never be
>      // associated with ruby text containers, hence this method should
>      // not be called in that case.
> +    MOZ_ASSERT(mBlockRS, "Should not call this method "
> +               "if there is no block reflow state available");

Bump the message all onto new line(s) after the condition, please.  Half-wrapping is just hard to read.

::: layout/style/StyleAnimationValue.cpp
@@ +417,5 @@
>                 posArray->Item(2).GetUnit() == eCSSUnit_Null,
>                 "Invalid list used");
>    for (int i = 0; i < 2; ++i) {
> +    MOZ_ASSERT(posArray->Item(i*2+1).GetUnit() != eCSSUnit_Null,
> +               "Invalid position value");

Mind putting spaces into i*2+1 while you're here?

@@ +427,5 @@
>    NS_ASSERTION(posArray->Item(0).GetUnit() == eCSSUnit_Null &&
>                 posArray->Item(2).GetUnit() == eCSSUnit_Null,
>                 "Invalid list used");
>    for (int i = 0; i < 2; ++i) {
> +    MOZ_ASSERT(posArray->Item(i*2+1).GetUnit() != eCSSUnit_Null,

And here.

::: layout/style/nsCSSValue.cpp
@@ +29,5 @@
>  nsCSSValue::nsCSSValue(int32_t aValue, nsCSSUnit aUnit)
>    : mUnit(aUnit)
>  {
> +  MOZ_ASSERT(aUnit == eCSSUnit_Integer || aUnit == eCSSUnit_Enumerated ||
> +             aUnit == eCSSUnit_EnumColor, "not an int value");

Bump the string to a new line?

@@ +375,5 @@
>  
>  void nsCSSValue::SetIntValue(int32_t aValue, nsCSSUnit aUnit)
>  {
> +  MOZ_ASSERT(aUnit == eCSSUnit_Integer || aUnit == eCSSUnit_Enumerated ||
> +             aUnit == eCSSUnit_EnumColor, "not an int value");

And here.

::: layout/style/nsStyleSet.cpp
@@ +1405,5 @@
> +                                 eRestyle_Force |
> +                                 eRestyle_ForceDescendants)),
> +             // FIXME: Once bug 979133 lands we'll have a better
> +             // way to print these.
> +             nsPrintfCString("unexpected replacement bits 0x%lX",

PRIX32, please, from IntegerPrintfMacros.h.

::: layout/svg/nsSVGFilterInstance.cpp
@@ +76,5 @@
>  
>    // Set the user space bounds (i.e. the filter region in user space).
>    nsSVGLength2 XYWH[4];
> +  MOZ_ASSERT(sizeof(mFilterElement->mLengthAttributes) == sizeof(XYWH),
> +             "XYWH size incorrect");

This should be a static_assert instead.

::: layout/svg/nsSVGOuterSVGFrame.cpp
@@ +438,1 @@
>        "We should have one anonymous child frame wrapping our real children");

Alignment of " underneath ! seems consistent with more code.

::: netwerk/base/nsSocketTransportService2.cpp
@@ +212,5 @@
>  
>  nsresult
>  nsSocketTransportService::AddToPollList(SocketContext *sock)
>  {
> +    MOZ_ASSERT(!(((uint32_t)(sock - mActiveList)) < mActiveListSize),

Mind making ((uint32_t)(...)) into a static_cast<uint32_t>(...) to get rid of some of the parentheses pileup here?  This is hard to read as is, seems best to combine nop changes into one commit.

@@ +265,5 @@
>  
>  nsresult
>  nsSocketTransportService::AddToIdleList(SocketContext *sock)
>  {
> +    MOZ_ASSERT(!(((uint32_t)(sock - mIdleList)) < mIdleListSize),

Same here, too.

::: parser/htmlparser/nsParser.cpp
@@ +756,1 @@
>      "The old parser is not supposed to be used for View Source anymore.");

Align up better?

::: view/nsView.cpp
@@ +816,5 @@
>  nsPoint nsView::GetOffsetTo(const nsView* aOther, const int32_t aAPD) const
>  {
> +  MOZ_ASSERT(GetParent() || !aOther || aOther->GetParent() ||
> +             this == aOther, "caller of (outer) GetOffsetTo must not "
> +             "pass unrelated views");

Move string to start on its own line?
Attachment #8558815 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8558817 [details] [diff] [review]
(part 2b) - Convert NS_ABORT_IF_FALSE to MOZ_ASSERT (single-line cases)

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

::: gfx/layers/TiledLayerBuffer.h
@@ +11,5 @@
>  
>  #include <stdint.h>                     // for uint16_t, uint32_t
>  #include <sys/types.h>                  // for int32_t
>  #include "gfxPlatform.h"                // for GetTileWidth/GetTileHeight
> +#include "nsDebug.h"                    // for MOZ_ASSERT

Can this be removed?  At the least, this comment is wrong now.

::: gfx/layers/ipc/ShadowLayerChild.cpp
@@ +7,5 @@
>  
>  #include "ShadowLayerChild.h"
>  #include "Layers.h"                     // for Layer
>  #include "ShadowLayers.h"               // for ShadowableLayer
> +#include "nsDebug.h"                    // for MOZ_ASSERT

Ditto for this being removable/fixable comment.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +27,5 @@
>  #include "ShadowLayerUtils.h"
>  #include "mozilla/layers/TextureClient.h"  // for TextureClient
>  #include "mozilla/mozalloc.h"           // for operator new, etc
>  #include "nsAutoPtr.h"                  // for nsRefPtr, getter_AddRefs, etc
> +#include "nsDebug.h"                    // for MOZ_ASSERT, etc

Same again.

::: gfx/src/nsFontMetrics.cpp
@@ +11,5 @@
>  #include "gfxPoint.h"                   // for gfxPoint
>  #include "gfxRect.h"                    // for gfxRect
>  #include "gfxTypes.h"                   // for gfxFloat
>  #include "nsBoundingMetrics.h"          // for nsBoundingMetrics
> +#include "nsDebug.h"                    // for NS_ERROR, MOZ_ASSERT

Comment needs fixing.  I would add a "mozilla/Assertions.h" include to deal with the other, rather than bootlegging it.

::: layout/ipc/RenderFrameParent.cpp
@@ +369,1 @@
>      // here will just cause the shadow subtree not to be rendered.

This should be rewrapped, slightly.

::: xpcom/glue/nsDebug.h
@@ +59,5 @@
>   * evaluate the expression argument. Hence side effect statements
>   * as arguments to the macro will yield improper execution in a
>   * non-debug build. For example:
>   *
> + *      MOZ_ASSERT(0 == foo++, "yikes foo should be zero");

No to the changes in this file, of course, which should just be removed in the followup patch to all the conversions.  :-)
Attachment #8558817 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8558818 [details] [diff] [review]
(part 2c) - Convert NS_ABORT_IF_FALSE to MOZ_ASSERT (fix-ups)

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

I would make NAIF removal a followup patch, separate from all conversions, myself.  Easier to back out if someone added a new use, don't have random blame-trampling on NAIF in the tree.

::: gfx/layers/TiledLayerBuffer.h
@@ +11,5 @@
>  
>  #include <stdint.h>                     // for uint16_t, uint32_t
>  #include <sys/types.h>                  // for int32_t
>  #include "gfxPlatform.h"                // for GetTileWidth/GetTileHeight
> +#include "nsDebug.h"                    // for NS_ASSERTION

Oh, you undo the weird bits here.  Carry on!

::: widget/gtk/nsWindow.cpp
@@ +6280,5 @@
>      }
> +#ifdef DEBUG
> +    // GDK_IS_WINDOW() expands to a statement-expression, and
> +    // statement-expressions are not allowed in template-argument lists. So we
> +    // have to make the MOZ_ASSERT condition indirect.

Alas.  I wonder if they could rewrite it for C++ as an immediately invoked lambda...
Attachment #8558818 - Flags: review?(jwalden+bmo) → review+
Somehow this seems to have caused a massive frequent intermittent leak in devtools, bug 1122249, on OSX 10.8.

I have no idea how that is possible, but that's what retriggers point to:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=70988272766a&tochange=8d8a696af76a&filter-searchStr=Rev5

This needs to get fixed or backed out.
Depends on: 1122249
Flags: needinfo?(n.nethercote)
This broke the build on my Linux machine (Fedora 21): 

 2:22.36 ../../../../../../dist/include/mozilla/Assertions.h:140: error: undefined reference to 'nsTraceRefcnt::WalkTheStack(_IO_FILE*)'
 2:22.36 collect2: error: ld returned 1 exit status
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
[14:59:36] <Waldo> mccr8: yeah, I'd back out at this point
[14:59:57] <Waldo> mccr8: probably we should try and binary-land the patch until we figure out what little bit is unhappymaking
Flags: needinfo?(n.nethercote)
I did a try run with all platforms and all tests before landing this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce438cdda7d0

Since this is intermittent, that doesn't guarantee it would have caught it. But *no tests were even run on Mac 10.8 debug* for that push. Looking at inbound, it seems that Mac 10.8 debug tests are skipped every few pushes, for no apparent reason. Sigh.
And I just did a bunch of Mac debug pushes to try but *none* of them are running tests on 10.8. Great.
Maybe it will work if you explicitly ask for 10.8 with something like -u all[10.8]?
(In reply to Nicholas Nethercote [:njn] from comment #25)
> And I just did a bunch of Mac debug pushes to try but *none* of them are
> running tests on 10.8. Great.

Looks like "try: -b d -p macosx64 -u mochitest-dt[10.8] -t none" is the magic incantation.
I've pushed a whole lot of try jobs and it's pretty clear that part 1 was the culprit, specifically the reworking of MOZ_ASSERT itself (i.e. *not* the ASSERT_UNLESS_FUZZING changes).

I have no idea why this caused problems, but given the leaks and the linking error in comment 21, I'm going to avoid changing MOZ_ASSERT. I will land all of part 2 except for the NS_ABORT_IF_FALSE calls that involve a string variable. I'll then change those cases like I did in the original patches. I'll post a new patch for these cases and get review before landing.
For the poisoning in nsPresArena.cpp I made it print out the details, because
that seems useful. For the other I simply removed the printing of the
unexpected value because that seems less important; we have countless
assertions like that elsewhere in the codebase that don't print the unexpected
value.
Attachment #8561779 - Flags: review?(jwalden+bmo)
(In reply to Nicholas Nethercote [:njn] from comment #29)
> For the poisoning in nsPresArena.cpp I made it print out the details, because
> that seems useful. For the other I simply removed the printing of the
> unexpected value because that seems less important; we have countless
> assertions like that elsewhere in the codebase that don't print the
> unexpected
> value.

This was a really useful quick debugging technique, and switching these to an assertion type that can't take nsPrintfCString() means that form of debugging will now take longer.

I'm fine with the style ones disappearing from the tree; those were mostly things that needed to be done to make one-off debugging easy.  But I'm substantially more upset to see the ability to do that easily removed.
(In reply to Nicholas Nethercote [:njn] from comment #24)
> Since this is intermittent, that doesn't guarantee it would have caught it.
> But *no tests were even run on Mac 10.8 debug* for that push.

Yes, as noted on Trychooser, 10.8 doesn't run by default on Try due to capacity issues (getting 24+hr backlogged on Try wasn't uncommon before that).

> Looking at inbound, it seems that Mac 10.8 debug tests are skipped every few pushes,
> for no apparent reason. Sigh.

There's always a reason ;). Test coalescing (if multiple jobs pending of the same type, only the one on the top push is run) is one. Also, debug tests run every 3rd push (subject to a max wait of 30min), again for capacity reasons. I know it sucks, but so do massive backlogs, which was the alternative.
https://hg.mozilla.org/mozilla-central/rev/20729b28eb1e
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whoops, there are more patches to land still.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
> This was a really useful quick debugging technique, and switching these to
> an assertion type that can't take nsPrintfCString() means that form of
> debugging will now take longer.
> 
> I'm fine with the style ones disappearing from the tree; those were mostly
> things that needed to be done to make one-off debugging easy.  But I'm
> substantially more upset to see the ability to do that easily removed.

By one-off debugging, do you mean cases where you are willing to temporarily modify the code so it prints the erroneous value? If so, using MOZ_ReportAssertionFailure() or even just fprintf(stderr) is just as good.
Attachment #8561779 - Flags: review?(jwalden+bmo) → review+
Blocks: 1134452
https://hg.mozilla.org/mozilla-central/rev/b4fa9c896fef
https://hg.mozilla.org/mozilla-central/rev/72481fdb3605
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.