Often the message argument to NS_ABORT_IF_FALSE is unhelpful (think non-null argument assertions). But sometimes it's useful, for assertions which are non-obviously correct or which are particularly complex. Variadic macros allow us to add an optional message to MOZ_ASSERT assertions. Sentiment on IRC and elsewhere wasn't opposed to adding an optional message (although it was adamantly opposed to requiring a message :-) ). MSVC 2005 (the oldest we care about) supports variadic macros, and gcc's supported them much longer, so I think we have the compiler support. There's at least one unconditional use of variadic macros in the tree, so theoretically any compiler that doesn't support them is broken already. But it can't hurt to be cautious, so let's start small. This patch makes MOZ_ASSERT take either one or two arguments and changes one file to use the two-argument form (that clearly wanted it) to ensure it works. We can let this sit for awhile, then go about announcing the change more broadly once nobody's complained, and making the other assertion macros take an optional message too. I asked Jesse about his preferred message format, since he's probably the biggest consumer of the current assertion message syntax, and he suggested this. This also happens to fit in the previous syntax, so it should be even more compatible. We're not sure what sort of message length will end up being used in practice, so we're going to play the exact format by ear. (Embedding the message this way might mean the typical fuzzbug that copies the assert into the summary might go past normal length limits, in theory. Whether this will happen, we'll only find out with greater use of it.)

Comment on attachment 586598 [details] [diff] [review] Patch with a few two-argument users >+ * MOZ_ASSERT(arg != NULL); We don't usually compare to null, so MOZ_ASSERT(arg)? And maybe fix the cases here: <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Database.cpp#1706> too?

Comparing to NULL is deliberate, because it's obvious to someone just skimming the example but not necessarily reading the flavor text that asserting not-null in that case doesn't need to be explained. I'm happy to fix more cases -- eventually. Right now I care about 1) does someone's obscure compiler break with this, and if so, do we care, and 2) does the fix make both forms work. Changing every place that should use the two-argument form isn't in-scope until it's clear the answer to #1 is no.

Comment on attachment 586598 [details] [diff] [review] Patch with a few two-argument users This is a spectacularly clever bit of cpp hackery. Kudos! However, I think this magic will potentially lead to confusion. Here are the scenarios I imagine // Oh cool! A variadic assert() //MOZ_ASSERT(cond1, cond2, cond3); // // Dammit. //test.cc:25: error: ‘MOZ_ASSERT_HELPERcond3’ was not declared in this scope // Well, ok. Two-cond assert. //MOZ_ASSERT(cond1, cond2); // // Dammit. //test.cc:33: error: expected ‘)’ before ‘cond2’ MOZ_ASSERT(!cond1 || (cond2 && cond3), "[Some complicated invariant explained]"); // Oh ... so MOZ_ASSERT requires an explanation. Okiedokie. MOZ_ASSERT(ptr, "ptr is not null"); I feel pretty strongly about not creating confusion over libc assert() any more than we already are with MOZ_ASSERT(). I think your solution here is really good for people used to NS_ASSERTION(), because it will be natural to think of the variadic assert as "explanation optional". But my intuition is that the potential for added confusion isn't worth the small notational gain, and indeed I think it might add incentives to write unclear assertions bailed out by the added message. My preference is to stick with MOZ_ASSERT() as is, and for folks who need to write crazy-complicated state assertions, use a style more like bool IsCrazyComplicated(...) { } MOZ_ASSERT(IsCrazyComplicated()); That is, use helper functions to explain the asserted state. But that said, I'm not against a MOZ_ASSERT() variant that takes a message. However I think it should be something like MOZ_ASSERT_EXPLAIN(cond, explanation). Feel free to solicit second and third opinions :).

Being used to NS_ASSERTION, I rather like the approach Waldo took :)

We can remove any mention of assert() from MOZ_ASSERT docs, for starters. The only real similarity is that they are both fatal, and we can make that clear without mentioning assert() and introducing that confusion. Once we do that, I'm not sure why someone would specifically identify MOZ_ASSERT with assert(), as opposed to general assertion functionality. Since this is clearly our macro, I'd think they'd expect what its documentation claims it provides and what they see existing uses doing. The intent of the message is not to explain *complicated* invariants, but rather *non-obvious* invariants. These would be invariants where, at first glance, you couldn't say why they were being made, or why they could be safely made. For example, consider this method in the JS engine now: void ClonedBlockObject::put(JSContext *cx) { StackFrame *fp = cx->fp(); JS_ASSERT(maybeStackFrame() == js_FloatingFrameIfGenerator(cx, fp)); uint32_t count = slotCount(); uint32_t depth = stackDepth(); /* The block and its locals must be on the current stack for GC safety. */ JS_ASSERT(depth <= uint32_t(cx->regs().sp - fp->base())); JS_ASSERT(count <= uint32_t(cx->regs().sp - fp->base() - depth)); /* See comments in CheckDestructuring in frontend/Parser.cpp. */ JS_ASSERT(count >= 1); copySlotRange(RESERVED_SLOTS, fp->base() + depth, count); /* We must clear the private slot even with errors. */ setPrivate(NULL); fp->setScopeChainNoCallObj(enclosingScope()); } The |count >= 1| assert is one that should have a message. Without the prior comment, it's completely non-obvious that all cloned block objects contain at least one binding because of a destructuring pattern hack implemented in the parser. The condition itself is not crazy-complicated at all. But it definitely requires an explanation. And an IsCrazyComplicated-style method doesn't make any sense for addressing the concern. Given that most people hate explaining assertions using NS_ABORT_IF_FALSE and NS_ASSERTION now, it seems unlikely to me that the pendulum would swing the other direction, with people explaining every assertion they make. And don't forget reviewer responsibility to only accept clean code. One reason people hate NS_ABORT_IF_FALSE is that it's long and unwieldy. Not using MOZ_ASSERT but rather something longer seems to start edging back in the long-and-unwieldy direction. And we want to make the barriers to use of the assertion macros as low as possible. I will continue soliciting opinions.

Is it particularly valuable to have the explanation show up in the runtime spew when the assert fails? Usually, I have to look at the source file/line anyway, so I would think that an explanation string has little value over a plain comment. On the subject of aesthetics, syntax highlighting will give explanation strings a different color than comments which is unpleasant. Also, multi-line string literals are gross so coders will feel inclined to cut their explanation shorter than if they used a comment. That is my solicited opinion :)

Well, there are also cases like this one, too, where the explanation clarifies what the assertion is checking: NS_ABORT_IF_FALSE(!mIPCOpen, "Attempt to retain more than one IPDL reference"); Or this one, which explains not just that cleanup's not needed, but that it's happening *twice*: NS_ABORT_IF_FALSE(!mNeedsCleanup, "double cleanup out of data frame"); Or something like this, which tells the developer to look at the locations for particular function calls he's written to find the culprit: NS_ABORT_IF_FALSE(!HasError(), "Shouldn't call WriteInternal after error!"); Further, such error messages print in tinderbox logs and such, for easier inspection in case a random orange happens to trigger one. You might also think of the explanations as helping to distinguish textually-similar assertions. The JS engine in particular with its naming consistency (sometimes to the point of foolishness, pn/pn2/pn3) would benefit from better disambiguation of similar assertion conditions by situation. We have multiple instances of JS_ASSERT(pnu->isUsed());, say, which are indistinguishable at a glance except by a line number that communicates nothing about the situation in which the assertion occurred.

(In reply to Jeff Walden (remove +bmo to email) from comment #7) All of those cases you gave could have just as well been comments except that they don't show up in logs. For fatal assertions (which I think is what we're talking about), I question the value of having the explanation string in logs: how much debugging happens at-a-glance without ever consulting the source or stack-trace? Perhaps I could see the value for but names but usually those are disambiguated (in the JS_ASSERT(!pn) cases you mentioned) in other ways that are more helpful anyway.

The debugging need not happen at-a-glance: an explanation might merely point the reader in the right direction faster, enabling him to skip looking at the location of the exception first. Every little bit helps. I occasionally recognize assertions I've written from fuzz bug title. Just as often, however, it turns out the assertion is a different assertion that's quite similar to the one I had in mind. An explanation would help resolve that remaining ambiguity, enabling me to perform my personal assertion triage of "should I investigate this particular assertion or leave it to other people" more quickly, without having to open up the relevant file to see source-and-line for it.

I gotta say, I'm totally with Waldo on this one. I'm *not* used to NS_ASSERTION, but when encountering any new assertion facility, I always check whether it allows (or requires) an explanation because it's a common facility and therefore a reasonable expectation. I would not expect multiple conditions because cond1 && cond2 && cond3 is obviously preferable to cond1, cond2, cond3. I want the actual string in the output because it's quicker to identify, allows for better bug titles and therefore searchability, and makes it easier to distinguish deep problems from superficial ones without digging into the code. And my syntax highlighter colors strings differently from code, which is good enough for me. This is also similar to things like assertions in tests, which really really need to have explanations with them (not always, but it should be available, and most should have them.) Which is admittedly a different situation, but similar enough for me to want them to be parallel.

(In reply to Jeff Walden (remove +bmo to email) from comment #9) I see your point; that is of non-zero value. Still, when weighed against the gross-ness of comments-in-string-literals (with the problems I listed above), I'm not convinced (although not adamantly opposed).

(In reply to Jeff Walden (remove +bmo to email) from comment #2) > Comparing to NULL is deliberate, because it's obvious to someone just > skimming the example but not necessarily reading the flavor text that > asserting not-null in that case doesn't need to be explained. I think the following example makes the intent clear: MOZ_ASSERT(aPointer) It would be unfortunate if the example leads people to think that MOZ_ASSERT(aPointer != NULL) is the preferred form when it's not. The first form is much faster for humans to parse, and to write, and it allows for a longer explanation on the same line when there is one.

(In reply to Jeff Walden (remove +bmo to email) from comment #5) > The intent of the message is not to explain *complicated* invariants, but > rather *non-obvious* invariants. Yes, you're right. I think "complicated" and "unhelpfully simple" are two sides of that coin. However, factoring out "unhelpfully simple" conditions into helpers isn't appealing, I conceded. And likely an onerous burden. > Given that most people hate explaining assertions using NS_ABORT_IF_FALSE and > NS_ASSERTION now, it seems unlikely to me that the pendulum would swing the other > direction, with people explaining every assertion they make. And don't forget > reviewer responsibility to only accept clean code. I doubt the majority of Gecko hackers know MOZ_ASSERT exists yet. If the "upgrade path" for NS_ABORT_IF_FALSE->MOZ_ASSERT is s/NS_ABORT_IF_FALSE/MOZ_ASSERT/, the ancestry of MOZ_ASSERT in assert() (i.e. allows just asserting the condition) may never fully disseminate and trivial messages might perpuate. But on the other hand, if the upgrade path for NS_ABORT_IF_FALSE->MOZ_ASSERT is dropping the NS_ABORT_IF_FALSE messages globally, then the counterargument is that we would drop a lot of meaningful messages and folks who don't watch .platform would never know that MOZ_ASSERT allowed writing them. So I guess an interesting datum is the approximate percentage of NS_ABORT_IF_FALSE (or maybe NS_ASSERTION?) messages that are nontrivial and should be preserved in some form, whether comment or string message. For NS_ABORT_IF_FALSE->MOZ_ASSERT we need to figure that out anyway, because NS_ABORT_IF_FALSE needs to die asap. (In reply to Steve Fink [:sfink] from comment #10) > I gotta say, I'm totally with Waldo on this one. I'm *not* used to > NS_ASSERTION, but when encountering any new assertion facility, I always > check whether it allows (or requires) an explanation because it's a common > facility and therefore a reasonable expectation. Personally I dread assertion mechanisms that require a message, because I know I'll end up writing trivial messages ad nauseam. But I've never used a "message optional" assertion. I haven't been fed a compelling argument yet but paragraph 4 of comment 5 is closest. The personal cost to me of perpetuated trivial assertions from everyone else in Gecko is basically zero. I've already got my (huge) personal benefit from cond-only MOZ_ASSERT, which this doesn't affect. There's likely non-zero general benefit to optional messages. The death of NS_ABORT_IF_FALSE benefits me, but there's a high cost of sorting the useful NS_ABORT_IF_FALSE messages into comments and discarding the trivial ones. So I guess the remaining issues are - "clean code": what defines a trivial message. Probably easy to sort out, and I don't personally care that much. - "I would think that an explanation string has little value over a plain comment", plus grossness: what's the cost? Folks who want to use comments to explain or think strings are gross can stick with comments, message optional. So I guess the potential cost is from inconsistent style. Really, both the remaining issues are over inconsistent style, I think. I'm not a style fascist so I'm ready to switch to r+, unless anyone raises objections tomorrow.

I went and made a few changes to the documentation comment based on discussion in previous comments, figure I may as well post it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/350305686094 I'll blog and newsgroup-post about this shortly.