Closed Bug 609210 Opened 9 years ago Closed 7 years ago
Compiler warning: Use of NS
_ENSURE _TRUE and NS _ENSURE _SUCCESS WITHOUT the second argument to it .
From the compilation of TB3 using comm-central, I found one disturbing set of warnings generated by the use of two-argument macros NS_ENSURE_TRUE and NS_ENSURE_SUCCESS WITHOUT the second argument, i.e., empty string to them. GCC generated the warning that the empty second argument to the macro will cause undefined behavior in ISO C preprocessor or to some such effect. (The exact wording in the case of NS_ENSURE_SUCCESS is like this: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98) I looked at such usages and found the following. NS_ENSURE_TRUE and NS_ENSURE_SUCCESS returns an error value (the second argument), if the expression in the first argument position signifies the error value. However, there are cases where NS_ENSURE_TRUE and NS_ENSURE_SUCCESS is being used in a void function. Obviously trying to return a value in a void function from NS_ENSURE_TRUE and NS_ENSURE_SUCCESS would cause a compilation error. So some programmers resorted to leaving the second argument as empty. In the existing CPP, this resulted in a simple "return ;" in the macro expansion, and the compilation didn't fail. This was certainly clever in the short term. But this is a very bad practice in my opinion. We should use a proper one-argument macro that does NOT return a value for such such situation to reduce the disturbing warnings. These extra warnings make it difficult to locate warnings that have REAL PROBLEMS behind them. Right now, TB3 compilation using comm-central produces tons of warnings :-( The attached is the fix the problem. It defines two macros: NS_ENSURE_TRUE_VOID, and NS_ENSURE_SUCCESS_VOID that take only a single argument and do not return a value. The patch replaces the bad usages of NS_ENSURE_TRUE and NS_ENSURE_SUCCESS with empty second argument by the use of these NS_ENSURE_TRUE_VOID and NS_ENSURE_SUCCESS_VOID to eliminate the disturbing warning messages. Patches for files only for TB3 in comm-central, and patches for ./mozilla subdirectory  patches for files under comm-central top directory, i.e., for TB3.  Patches for files under ./mozilla subdirectory. A few fixes for uninitialized variable and ununsed variable crept in. I am not sure what the correct initial value should be for the next one. Maybe NS_FALSE? --- a/mailnews/imap/src/nsImapMailFolder.cpp Tue Nov 02 10:34:06 2010 +0000 ... the following initial value may have to be PR_FALSE? + nsresult rv = 0; // initial value that is returned sometimes!
This is the patch for the TB3 files under comm-central.
After fixing other bugs and rebuild TB3, I found the same problems are found in the following files under ./mozilla subdirectory. So I upload the more comprehensive patches. diff -r b192fa5cfffc accessible/src/base/nsEventShell.cpp diff -r b192fa5cfffc accessible/src/html/nsHTMLFormControlAccessible.cpp diff -r b192fa5cfffc accessible/src/xul/nsXULFormControlAccessible.cpp diff -r b192fa5cfffc content/html/content/src/nsTextEditorState.cpp diff -r b192fa5cfffc content/html/document/src/nsHTMLDocument.cpp diff -r b192fa5cfffc dom/indexedDB/IDBTransaction.cpp diff -r b192fa5cfffc dom/src/threads/nsDOMThreadService.cpp diff -r b192fa5cfffc editor/libeditor/html/nsHTMLEditor.cpp diff -r b192fa5cfffc layout/base/nsCaret.cpp diff -r b192fa5cfffc layout/generic/nsObjectFrame.cpp diff -r b192fa5cfffc layout/style/nsHTMLStyleSheet.cpp diff -r b192fa5cfffc xpcom/glue/nsDebug.h
Attachment #487841 - Attachment is obsolete: true
After fixing other problems, and rebuilding TB3 using comm-central, I now realize the same problems are found in the following files and so I am uploading the complete patch for TB3 files in comm-central. diff -r 55086537a691 mailnews/addrbook/src/nsAbAddressCollector.cpp diff -r 55086537a691 mailnews/base/src/nsMsgContentPolicy.cpp diff -r 55086537a691 mailnews/base/src/nsMsgMailSession.cpp diff -r 55086537a691 mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp diff -r 55086537a691 mailnews/imap/src/nsAutoSyncManager.cpp diff -r 55086537a691 mailnews/imap/src/nsImapMailFolder.cpp diff -r 55086537a691 mailnews/local/src/nsLocalMailFolder.cpp I wonder if I should separate the TB3 patch and ./mozilla patch into diffrent bugzilla entries. In any case, the problem appears simply in too many files, but the files list is now complete.
Attachment #487843 - Attachment is obsolete: true
Are we getting this warning in current tip of m-c as well? I didn't see any such warning in my build logs of firefox (using gcc 4.6.1 on linux).
Given that there are still the usage of NS_ENSURE_TRUE macro without any second arguments, I suspect that the compiler warning options have been changed? Searching such occurrences using mxr.mozilla.org in common-central code which should have mozilla-central code under it, I still see some such occurrences: e.g., mozilla/content/base/src/nsEventSource.cpp (View Hg log or Hg annotations) line 881 -- NS_ENSURE_TRUE(mHttpChannel, NS_ERROR_NO_INTERFACE); line 1179 -- NS_ENSURE_TRUE(srcToTest, PR_FALSE); line 1315 -- NS_ENSURE_TRUE(mMessagesToDispatch.GetSize() == sizeBefore + 1, line 1348 -- NS_ENSURE_TRUE(sgo,); line 1351 -- NS_ENSURE_TRUE(scriptContext,); line 1354 -- NS_ENSURE_TRUE(cx,); line 1368 -- NS_ENSURE_TRUE(jsString,); So if the compilation doesn't raise any warnings to these, then it is likely that the warning setting of the compiler may have been changed. I also found an interesting code: mozilla/security/manager/ssl/src/nsCertTree.cpp (View Hg log or Hg annotations) line 1485 -- NS_ENSURE_TRUE( (cert!=0 && entry!=0), RETURN_NOTHING ); line 1540 -- NS_ENSURE_TRUE( (a!=0 && ace!=0 && b!=0 && bce!=0), 0 ); Here RETURN_NOTHING is defined by pre-processor to nothing. At least the coder was trying to be honest about it, but this should also be fixed IMHO. BTW, maybe 4.6.1 doesn't warn on this problematic construct by default?
Hi, I am trying to fix this for mailnews in bug 716278. Did you come to any good method to fix these warnings? Is the RETURN_NOTHING solution working? I can confirm the warnings were there with gcc 4.5 just recently it the time of filing bug 716278, but I also can't see them today. What happened?
Depends on: 716278
For a possible solution, please see the introduction of a new macros in xpcom/glue/nsDebug.h in my first attachment above: https://bugzilla.mozilla.org/attachment.cgi?id=488481 I introduced a macro WITHOUT the second argument when the function in question has void return type. The rest is to replace the offending code left and right, which may not be liked much by module owners, but I think we should strive for an honest code that sticks to defined behavior of preprocessor and macro. I am not sure what happened to the GCC compiler now. Maybe the warning message has been changed? (I am using GCC 4.6.1 now, I think.) Use of RETURN_NOTHING is not recommended, and if we add RETURN_NOTHING to everywhere, we might as well rewrite the offending code using the newly introduced macros everywhere as well. Just my idea.
I would gladly use the new 1 argument macros if you can push them through the module owners. Have you tried reviews recently? (I don't know what is with the warnings, I did not change the compiler (4.5.3) and they do not show anymore.)
(In reply to :aceman from comment #8) > I would gladly use the new 1 argument macros if you can push them through > the module owners. > > Have you tried reviews recently? > > (I don't know what is with the warnings, I did not change the compiler > (4.5.3) and they do not show anymore.) I have not tried review yet since the problem seems to be found in many files scattered all over the directories. (But maybe it is the xpcom/glue/nsDebug.h, i.e., some type of debug support module or whatever?) Any idea who to put in the reivewer's field on this matter? (Or can I simply set a request for a review and without specifying who?)
I think the main problem is to get the macro definition into xpcom/glue/nsDebug.h . I think one of Benjamin Smedberg, Doug Turner, Mike Shaver would be good for reviewing that reduced patch. (It is not good to kae a review request without specifying a name.) Because you didn't get a comment yet from any mozilla developer with any say on this, if this change is even acceptable. Only once that is done we should try to find all the usages of the current macro (where the the second argument is missing) and make some patches, potentially splitting them per mozilla component. It is not sure those 3 people would like to review those patches for the various scattered files.
As I noted on the newsgroups, I am opposed to NS_ENSURE_* in general, and so I oppose this change. I would rather you spend time on implementing if (NS_WARN_IF_FAILED(rv)) return;
What is the problem with NS_ENSURE_* please?
It hides control flow (return statements).
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 788242
You need to log in before you can comment on or make changes to this bug.