Closed Bug 672843 Opened 8 years ago Closed 6 years ago

Create new macros and inline functions to replace NS_ENSURE* macros

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: sfink, Assigned: benjamin)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

Come up with a new set of macros to replace NS_ENSURE*. (Leave the existing ones for backwards compatibility for a while.)

We never completely settled on the final names. Which may mean that this bug will just rot away into oblivion. Currently, my script produces:

NS_ENSURE_TRUE(cond,rv)     -> if (MOZ_WARN_IF(!cond))
                                   return rv;
NS_ENSURE_FALSE(cond,rv)    -> if (MOZ_WARN_IF(cond))
                                   return rv;
NS_ENSURE_SUCCESS(res,ret)  -> if (MOZ_WARN_IF_FAIL(res))
                                   return ret;

and the various NS_ENSURE_SPECIFIC_TEST macros get rewritten similarly to NS_ENSURE_TRUE, except the error return values are different (the macros weren't really helping clarify much imho). So for example:

NS_ENSURE_ARG(cond)          -> if (WARN_IF(!cond))
                                    return NS_ERROR_INVALID_ARG;

That specific rewriting is just a proposal. I do not intend to drive this bug to conclusion right now (or any time soon), but if someone else comes up with a patch to nsDebug.h with a specific set of macros, I'll do the rewrite script (or help someone else do it if they want to do it instead.)
OS: Linux → All
Hardware: x86_64 → All
Code style requires curlies. Also, can we spend time on useful bugs, rather than bitrotting roughly all patches in existence for doubtful gains?
(In reply to comment #1)
> Code style requires curlies.

Consensus on the thread was that this was an acceptable style deviation. It occurs frequently enough that the change in code density would be worse than the inconsistent bracing style.

> Also, can we spend time on useful bugs,

I won't argue this, but there is a large contingent of people who believe this rewrite would be useful. Personally, I will be working on other bugs until a miracle happens and consensus is achieved on the final rewritten form. (I mostly filed these bugs to archive the state of the discussion rather than let it be completely lost in the history of dev-platform.)

> rather than bitrotting roughly all patches in existence for doubtful gains?

The old macros will be left in place for this reason.

I forgot to put a link into a sample (outdated) rewrite at http://people.mozilla.org/~sfink/uploads/rewrite3.diff.gz
Lets go with s/NS_ENSURE_SUCCESS/if(NS_FAILED_W(rv)) return rv/ to begin with, can take care of the others once we reach consensus and land this.
I don't see consensus for that either.
(In reply to Ms2ger from comment #4)
> I don't see consensus for that either.

We'll go through that during the review process.
Note the macro name should be MOZ_FAILED_W. NS is a silly prefix indeed.
(In reply to Taras Glek (:taras) from comment #6)
> Note the macro name should be MOZ_FAILED_W. NS is a silly prefix indeed.

Having to remember which half of our macros use NS_ and which half use MOZ_ is worse (NS_FAILED vs. MOZ_FAILED_W, seriously?).  If we have a plan to convert completely, fine; otherwise I favor sticking with NS_.

Also, when I see NS_FAILED_W, I think it means "failed with".  Given the rest of the conversion this is doing, I tend to think having a macro to condense NS_WARN_IF(NS_FAILED(...)) into a single pair of parentheses isn't worthwhile.
(In reply to David Baron [:dbaron] from comment #7)
> (In reply to Taras Glek (:taras) from comment #6)
> > Note the macro name should be MOZ_FAILED_W. NS is a silly prefix indeed.
> 
> Having to remember which half of our macros use NS_ and which half use MOZ_
> is worse (NS_FAILED vs. MOZ_FAILED_W, seriously?).  If we have a plan to
> convert completely, fine; otherwise I favor sticking with NS_.

I think there is value in converting over existing NS_FAILED macros to warn. However in the interest of doing one big change at a time I would rather expand NS_ENSURE_SUCCESS macro and then go from there.

We'll take whatever name people prefer just before landing. No point blocking actual work on naming issues, that can be done in parallel.
(In reply to Taras Glek (:taras) from comment #8)
> I think there is value in converting over existing NS_FAILED macros to warn.

I don't (in case you thought I did).
As module owner of XPCOM, I believe dbaron's form is the best: NS_WARN_IF(NS_FAILED(foo)). And we must absolutely not make NS_FAILED warn by default: failure codes are perfectly normal and do not warrant warnings in much of our code.
(In reply to Benjamin Smedberg  [:bsmedberg] (Away 24-25 October) from comment #10)
> As module owner of XPCOM, I believe dbaron's form is the best:
> NS_WARN_IF(NS_FAILED(foo)). 

Has anything come out of this discussion?

> And we must absolutely not make NS_FAILED warn
> by default: failure codes are perfectly normal and do not warrant warnings
> in much of our code.

I wholeheartedly agree with this. I am analyzing |make mozmill| test suite run of local debug build of thunderbird. The log is full of warnings, 
 - some of which seem to indicate serious errors, but
 - many of which seem to be just noise :-(

The noise to signal ratio is not good and so I have written a script to
pick up interesting lines (but this is white list, and is not complete. I have to
scan the log occasionally to add missed lines to the white list.)

TIA
Attachment #823508 - Flags: review? → review?(nfroyd)
Comment on attachment 823508 [details] [diff] [review]
Create the new macro NS_WARN_IF and deprecate NS_ENSURE_* in favor of the explicit warning/return style. While I'm touching it, localize each macro so that it's debug and non-debug versions are near eachother, because that makes it easier for new contribu

Wrong patch.
Attachment #823508 - Attachment is obsolete: true
Attachment #823508 - Flags: superreview?(jst)
Attachment #823508 - Flags: review?(nfroyd)
Comment on attachment 823510 [details] [diff] [review]
Create the new macro NS_WARN_IF and deprecate NS_ENSURE_* in favor of the explicit warning/return style. While I'm touching it, localize each macro so that it's debug and non-debug versions are near eachother, because that makes it easier for new contribu

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

Maybe move the "While I'm touching it..." bit out of the header line of the commit into the description.  Also, typo in "are near eachother" (needs a space).

NS_WARN_IF(NS_FAILED(rv)) sounds like a mouthful, but maybe the sheer pain of it will convince people to not warn unless it's really necessary.

::: xpcom/glue/nsDebug.h
@@ +74,5 @@
>      if (!(_expr)) {                                           \
>        NS_DebugBreak(NS_DEBUG_ABORT, _msg, #_expr, __FILE__, __LINE__); \
>      }                                                         \
>    } while(0)
> +#else 

Nit: trailing whitespace.

@@ +275,5 @@
>  #define NS_RUNTIMEABORT(msg)                                    \
>    NS_DebugBreak(NS_DEBUG_ABORT, msg, nullptr, __FILE__, __LINE__)
>  
>  
>  /* Macros for checking the trueness of an expression passed in within an 

While you're here, can you nuke this trailing whitespace?
Attachment #823510 - Flags: review?(nfroyd) → review+
NS_ENSURE_PROPER_AGGREGATION was only used by nsAgg.h, so I just moved it.
Attachment #824038 - Flags: review?(nfroyd)
Attachment #824038 - Flags: review?(nfroyd) → review+
Attachment #823510 - Flags: superreview?(jst) → superreview+
Comment on attachment 824035 [details] [diff] [review]
This patch is *not* auto-generated. I did it partly by hand and partly using emacs macros to see how hard it would be. While I was in there, I removed some checks which are no longer needed because of infallible new and some checks for null outparams whic

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

What about nsLocalFileOS2.cpp and the files in xpcom/tests/?
Your commit description got truncated.. you dropped all checks for NULL-ness of out pointers? But you left them in for _retvals?

::: xpcom/build/nsXPComInit.cpp
@@ +501,1 @@
>          

Let's clean up the extra spaces while we're at it

::: xpcom/ds/nsWindowsRegKey.cpp
@@ +142,5 @@
>    DWORD numSubKeys;
>    LONG rv = RegQueryInfoKeyW(mKey, nullptr, nullptr, nullptr, &numSubKeys,
>                               nullptr, nullptr, nullptr, nullptr, nullptr,
>                               nullptr, nullptr);
> +  if (rv != ERROR_SUCCESS)

Why not NS_WARN_IF when checking non-nsresult returns?

::: xpcom/io/nsAnonymousTemporaryFile.cpp
@@ +67,3 @@
>    rv = tmpFile->Create(nsIFile::DIRECTORY_TYPE, 0700);
> +  if (rv != NS_ERROR_FILE_ALREADY_EXISTS && NS_WARN_IF(NS_FAILED(rv)))
> +    return rv;

Nice short-circuit :)

::: xpcom/io/nsBinaryStream.cpp
@@ +255,4 @@
>      nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aObject);
> +
> +    // Can't deal with weak refs
> +    if (NS_WARN_IF(!aIsStrongRef) || NS_WARN_IF(!classInfo) ||

!aIsStrongRef should probably return NS_ERROR_UNEXPECTED instead of NS_ERROR_NOT_AVAILABLE

@@ +803,4 @@
>  
>      rv = serializable->Read(this);
> +    if (NS_WARN_IF(NS_FAILED(rv)))
> +        return rv;    

Trailing space

::: xpcom/threads/LazyIdleThread.cpp
@@ +156,5 @@
>    }
>  
>    mIdleTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
> +  if (NS_WARN_IF(!mIdleTimer))
> +    return NS_ERROR_UNEXPECTED;

You changed the error codes returned from the functions in this file from NS_ERROR_FAILURE to NS_ERROR_UNEXPECTED?

::: xpcom/threads/nsThread.cpp
@@ +428,5 @@
>    if (!mThread)
>      return NS_OK;
>  
> +  if (NS_WARN_IF(mThread == PR_GetCurrentThread()))
> +    return NS_ERROR_NOT_SAME_THREAD;

The new error code returned here seems a bit odd, since this check ensures that nsThread::Shutdown was *NOT* called from the same thread

@@ +482,5 @@
>  NS_IMETHODIMP
>  nsThread::HasPendingEvents(bool *result)
>  {
> +  if (NS_WARN_IF(PR_GetCurrentThread() != mThread))
> +    return NS_ERROR_NOT_SAME_THREAD;

It makes sense here
Attachment #824035 - Flags: review?(vdjeric) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/78fb435aa0d2 for xpcshell and mochitest-5 orange
Could this fix have broken the add-on Stylish?  I'm trying to narrow the fix that broke Stylish and this is one candidate.  I'll see if the backout fixes the problem later on.
Just updated with cset https://hg.mozilla.org/integration/mozilla-inbound/rev/78fb435aa0d2 and Stylish works fine again.
Depends on: 942383
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.