859197 - nsMsgSendReport.cpp:264:7: warning: case value '2153066710' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]

Status: RESOLVED DUPLICATE of bug 783526

(Thunderbird :: Untriaged, defect)

Description

[ISHIKAWA, Chiaki]

I noticed a series of strange warning lines during compilation of thunderbird 
(comm-central) using gcc-4.7.

The warnings are:

nsMsgSendReport.cpp
/COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp: In member function 'virtual nsresult nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool, nsresult*)':
/COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:264:7: warning: case value '2153066710' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]
/COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:263:7: warning: case value '2153066711' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]
/COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:259:7: warning: case value '2153066728' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]
/COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:262:7: warning: case value '2153066733' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]
/COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:261:7: warning: case value '2153066740' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]
/COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgSendReport.cpp:260:7: warning: case value '2153066768' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]


The offending code that produced warnings are as follows:
The lines marked with "*" at the beginning of line produced warnings.

  //Do we have an explanation of the error? if no, try to build one...
  if (currMessage.IsEmpty())
  {
    switch (currError)
    {
      case NS_BINDING_ABORTED:
*     case NS_ERROR_SEND_FAILED:
*     case NS_ERROR_SEND_FAILED_BUT_NNTP_OK:
*     case NS_MSG_FAILED_COPY_OPERATION:
*     case NS_MSG_UNABLE_TO_SEND_LATER:
*     case NS_MSG_UNABLE_TO_SAVE_DRAFT:
*     case NS_MSG_UNABLE_TO_SAVE_TEMPLATE:
        //Ignore, don't need to repeat ourself.
        break;
      default:
        nsMsgGetMessageByID(currError, currMessage);
        break;
    }
  }


These case label values are defined in the following.
https://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsComposeStrings.h#42

These are NOT defined in nsError.h, or
ErrorList.h which is included in nsError.h as it turned out.
https://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/nsError.h
https://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/ErrorList.h

I see that the file in question (nsComposeStrings.h) defines internal operation codes and some values  other than error codes as well:
I think the numbers that are defined by using NS_MSG_GENERATE_FAILURE() macro
are error codes.
However, there is a lapse. A few error codes are defined without the
use of NS_MSG_GENERATE_FAILURE, it seems.
44 #define NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SAVING    12533

There is something fishy going on here.
I think error codes should be defined in ErrorList.h which are included
in nsError.h. And that is what the compiler is complaining about.
|enum tag_result| which is the original type of typedef'ed |nsresult|
will not include  the values in the quoted case values in warning lines unless
these values are defined in ErrorList.h as well.

I think there is serious oversight in the organization of error codes in
thunderbird which manifests itself in the compiler warning.
(Maybe the idea was to use a localized error code to a module or something, but
there is already a conflict of a sort. See the comment:
https://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsComposeStrings.h#57

/* 12554 is taken by NS_ERROR_NNTP_NO_CROSS_POSTING.  use 12555 as the next one */


Such a strange conflict would have been avoided or at least noticed
earlier (hopefully) by the time of registering it in ErrorList.h
(???)

Anyway, the warnings are disconcerting enough and
will be a burden on the future maintainers and
should be eliminated by
 - registering the error values properly in ErrorList.h, or
 - any other method deemed appropriate.

In the long future, compiler may even decide to ignore (i.e., not
generate) these case branches thinking since the values won't happen
in a strictly conforming code, and at the same time refuses to return
the value as a function value of type nsresult even.

So we should move the error values to more visible shared
file.

Since NS_ERROR_MODULE_MAILNEWS is defined as 16 in nsError.h
https://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/nsError.h#51

50 #define NS_ERROR_MODULE_IMGLIB     15
51 #define NS_ERROR_MODULE_MAILNEWS   16
52 #define NS_ERROR_MODULE_EDITOR     17

I think we can define these values in the following position (marked
with <====) between the values for NS_ERROR_MODULE_IMGLIB and
NS_ERROR_MODULE_EDITOR in ErrorList.h:

Excerpt from ErrorList.h:

506   /* ======================================================================= */
507   /* 15: NS_ERROR_MODULE_IMGLIB */
508   /* ======================================================================= */
509 #define MODULE NS_ERROR_MODULE_IMGLIB
510   ERROR(NS_IMAGELIB_SUCCESS_LOAD_FINISHED,  SUCCESS(0)),
511   ERROR(NS_IMAGELIB_CHANGING_OWNER,         SUCCESS(1)),
512 
513   ERROR(NS_IMAGELIB_ERROR_FAILURE,       FAILURE(5)),
514   ERROR(NS_IMAGELIB_ERROR_NO_DECODER,    FAILURE(6)),
515   ERROR(NS_IMAGELIB_ERROR_NOT_FINISHED,  FAILURE(7)),
516   ERROR(NS_IMAGELIB_ERROR_NO_ENCODER,    FAILURE(9)),
517 #undef MODULE
518                       
519                                     <=========== **** HERE *****
520   /* ======================================================================= */
521   /* 17: NS_ERROR_MODULE_EDITOR */
522   /* ======================================================================= */

What do people think?

There may have been a desire to decouple the codes from thunderbird in XPCOM error codes, but as I see it, nsError.h already includes the base value for Calendar, thunderbird and a few other things, I think what I describe above is
a prudent thing to do. 


OK, after having written up to this sentence, I have found out that
msgCore.h claims to be the central repository of error codes for mailnews (!?):
https://mxr.mozilla.org/comm-central/source/mailnews/base/public/msgCore.h#101

101 /* This is where we define our errors. There has to be a central
102    place so we don't use the same error codes for different errors.
103 */

Indeed. Then I found more cryptic messages:

163 /* Error codes for message compose are defined in
164    compose\src\nsMsgComposeStringBundle.h. Message compose use the same error
165    code space as other mailnews modules. To avoid any conflict, values between
166    12500 and 12999 are reserved.
167 */
168 #define NS_MSGCOMP_ERROR_BEGIN 12500
169 /* NS_ERROR_NNTP_NO_CROSS_POSTING lives here, and not in nsMsgComposeStringBundle.h, because it is used in news and compose. */
170 #define NS_ERROR_NNTP_NO_CROSS_POSTING NS_MSG_GENERATE_FAILURE(12554)
171 #define NS_MSGCOMP_ERROR_END 12999
(I wonder if this was before mail and news are merged???)

So even within thunderbird files, there seems to be a confusion.

I wonder if people in the know can explain the historical reason for the current strange state of affairs. (Not listing the error codes for news and compose in a central place in thuderbird. I don't see any currently important reasons for splitting the error codes in different files for news, mail, and compose(?))

Oh, maybe so called SpiderMonkey has some issues with merged error code space?

The codes defined in the central position for mailnews (i.e., thunderbird's msgCore.h) should then be moved to ErrorList.h eventually before compilers decide to do funny things for values not defined for |enum tag_result|.
(We can include nsError.h from msgCore.h)

I think we do need to clean up the current state one way or the other before 
the compiler may decide to do funny things (read aggressively optimized) for run-time value not listed in enum type definition in the long run.

TIA

PS: As I checked the error values, I found this nugget that needs fixing, too.
But that will be another bugzilla entry.

https://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#2621


2620           mSendReport->SetError(nsIMsgSendReport::process_Current,
2621               // XXX The following applies NS_ERROR_GENERATE_FAILURE twice,
2622               // which doesn't make sense.  Just NS_MSG_ERROR_ATTACHING_FILE is
2623               // surely what was intended.
2624               NS_ERROR_GENERATE_FAILURE(
2625                 NS_ERROR_MODULE_MAILNEWS,
2626                 static_cast<uint32_t>(NS_MSG_ERROR_ATTACHING_FILE)),
2627               false);

Comment 1

[Magnus Melin [:mkmelin]]

See bug 783526?

Comment 2

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin from comment #1)
> See bug 783526?

Right on! It just does not show up when I searched. 

Anyway, compiler is getting cleverer and pickier.

I will follow 785326 and probably file a separate bug for the "XXX" case above.

Once the movement of constants to nsError.h (ErrorList.h) is decided
in its favor or the other way in 783526, I can figure out what to do.
(Well, actually, if the constants are not moved to nsError.h or ErrorList.h,
I have no idea how to shut this up. -Wswitch seems to be such a powerful static 
debug aid, and I am not sure if we want to disable it. [Maybe in this section only using
#pragma? ]
These will be discussed I think in 785326, too.

Thank you again.