I have to say, I found plenty of bugs and other weird stuff in m-c when converting nsresult to enum, but so far, c-c is vastly more horrible. You don't have this kind of rampant abandon in interchanging integer types and nsresult anywhere in m-c.

Ha! Though, given mailnews's history, it really shouldn't be much of a surprise.

FWIW, some of the code was mostly directly copied from the NS 5 version of mozilla and predates the use of nsresult codes and instead uses the "negative result for error codes." Replacing this stuff fell into the category of "fix it when you're already mucking around there," so it never was a particularly big priority to fix.

Apparently successful try run for the series I'm posting now: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=76b8cfcd1809 All failures starred, or successful on retrigger. I believe every single change in this patch is just switching between 0, NS_OK, nullptr, and false. In some cases this might actually be indicative of an underlying bug, such as if a function tries to return false (indicating error) but actually returns NS_OK (indicating success). But in any event, this patch shouldn't change any behavior. Let me know if you're the right person to ask to review this stuff.

We have a typedef; let's use it, please. This is used by later patches, because MimeConverterOutputCallback is defined to return nsresult, but actual functions of that type really return int, so I want to change the typedef to int in one fell swoop. In this patch I also change mime_output_fn() and mime_multipart_related_output_fn() to return nsresult instead of int so they match the new MimeConverterOutputCallback definition. This is to avoid compiler errors for returning signed vs. unsigned, since at this point nsresult is still PRUint32. In a later patch I'll change them back.

You can't do nsresult rv = Foo(); rv |= Bar(); when nsresult is an enum. This uses additional variables instead. Or, if there are many such |= in a row or rv is returned, it makes rv contain the last failure code.

This changes lots of int, PRUint32, etc. to nsresult -- variables, return values, parameters, and so on. I tried to make a judgment call in each case as to whether it was used more like an nsresult or integer, but sometimes it was used as both. Then I picked one arbitrarily, and added casts to avoid eventual compilation errors. Changing PRUint32 to nsresult should have no consequence at all except added type safety (once nsresult is an enum). However, changing int to nsresult makes it unsigned, which can change behavior -- e.g., "if (status < 0)" has to be changed to "if (NS_FAILED(status))". I tried to check for this.

In some cases, something is just used as the wrong type, and I saw no easy way to fix it. In these cases, there was no easy option other than just adding a cast. This patch shouldn't change behavior, but it highlights dubious code so that at least the person reading it will hopefully notice. In a lot of cases the intent seems clear, but I don't want to risk changing behavior. In many cases it's just "static_cast<nsresult>(-1)", which could probably be safely changed to "NS_ERROR_FAILURE" or something. I can't realistically spend the time to fix many of these, particularly since I don't know the code well. (The result of NS_ERROR_GENERATE_FAILURE probably shouldn't need to be cast, but my current patches for m-c require it to be. If I wind up changing that, these parts of the patchset can be dropped.)

This is the inverse of part 4. In cases where I judged values weren't really being used as nsresults, I changed the type to something else.

In some cases, I was able to avoid casting nsresults by just removing them entirely. This includes nsresults that are never checked, or that are only checked in a way where the result is always the same (e.g., assigning a boolean to an nsresult and then checking NS_FAILED). In EnumerateCards(), a bool was being used as an nsresult, so the NS_FAILED() call would always return false. I changed it here to match the intent, because it was only used in an assertion, so it doesn't change behavior in production. (I'm avoiding actual changes to behavior across the board here, because I'm changing so many things at once and I don't understand them.) In the case of DeleteSmtpServer, I verified that there were no C++ callers, so changing it to always return NS_OK instead of a boolean-converted-to-nsresult won't change anything (no exception either way). These seven patches fix all the nsresult misuse that's picked up by making nsresult an enum. Making it an enum class will doubtless find more -- I'm still working on that patch set for m-c.

Comment on attachment 650090 [details] [diff] [review] Patch part 1 -- Fix type of constants Ok, I'm going to punt some of these in irving's direction, and I'll look at the rest soon.

Comment on attachment 650094 [details] [diff] [review] Patch part 2 -- Use MimeConverterOutputCallback more Review of attachment 650094 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, much cleaner. I'd probably comment about improving some of the style on the lines you're changing, however, as this is large amounts of replacement, and it was all bad before, I think we can skip that this time.

Yes, generally I fix style on code I change, but not when I'm changing so much at once. Especially because it can make extremely long, repetitive patches a lot harder to review.

Comment on attachment 650105 [details] [diff] [review] Patch part 7 -- Remove unnecessary nsresults Review of attachment 650105 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsSmtpService.cpp @@ +669,5 @@ > aServer->ClearAllValues(); > > mServerKeyList = newServerList; > saveKeyList(); > + return rv; You removed the definition of rv above, so this is actually broken. I'd suggest just leaving the return NS_OK as that's what we've been doing up until now.

Comment on attachment 650101 [details] [diff] [review] Patch part 5 -- Add lots of static_cast<nsresult> Review of attachment 650101 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/addrbook/src/nsAbLDAPListenerBase.cpp @@ +337,5 @@ > } > > // Don't know how to handle this, so use the message error code in > // the failure return value so we hopefully get it back to the UI. > + return static_cast<nsresult>(NS_ERROR_GENERATE_FAILURE( Isn't NS_ERROR_GENERATE_FAILURE already cast to nsresult? ::: mailnews/compose/src/nsMsgSendPart.cpp @@ +234,5 @@ > if (m_encoder_data) > { > nsresult rv = MIME_EncoderWrite(m_encoder_data, encoded_data, length); > if (NS_FAILED(rv)) > + status = static_cast<nsresult>(-1); Can you add XXX statements for these as well please (several places in the patch)?

(In reply to Mark Banner (:standard8) from comment #14) > You removed the definition of rv above, so this is actually broken. I'd > suggest just leaving the return NS_OK as that's what we've been doing up > until now. Oops! Dunno how that snuck in -- it wasn't in my push to try (obviously).

Parts 2 and 7: https://hg.mozilla.org/comm-central/rev/78f676e4f2d3 https://hg.mozilla.org/comm-central/rev/3285912ee959

Comment on attachment 650090 [details] [diff] [review] Patch part 1 -- Fix type of constants Review of attachment 650090 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsSmtpService.cpp @@ +669,5 @@ > aServer->ClearAllValues(); > > mServerKeyList = newServerList; > saveKeyList(); > + return NS_OK; Ignore this change -- it was already pushed as part of a separate patch to fix build breakage: https://hg.mozilla.org/comm-central/rev/7387d34b4335

Comment on attachment 651679 [details] [diff] [review] Part part 5, v2 -- Add lots of static_cast<nsresult> Review of attachment 651679 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. In case you hadn't realised, this patch does depend on the other ones in this bug.

I'll hold off on pushing any more patches until they're all reviewed, then. No rush.

- rv = m_readSet->Add(messageKey); - if (NS_FAILED(rv)) return false; + if (m_readSet->Add(messageKey) < 0) return false; Why is this change good? Isn't NS_FAILED() preferred and future-proof?

This may also help bug 736766.

(In reply to :aceman from comment #23) > - rv = m_readSet->Add(messageKey); > - if (NS_FAILED(rv)) return false; > + if (m_readSet->Add(messageKey) < 0) return false; > > Why is this change good? Isn't NS_FAILED() preferred and future-proof? Yes, if rv is actually an nsresult. In this case, the function returns an int: http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgKeySet.cpp#641 If the function were changed to return nsresult, which it probably should be, NS_FAILED() would be correct. But I'm not trying to clean up all the functions that sometimes return int and sometimes nsresult; that would add a lot of work to an already fairly large job.

Thanks.

Comment on attachment 650090 [details] [diff] [review] Patch part 1 -- Fix type of constants Review of attachment 650090 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, I'll attach a separate patch that fixes all the places where we were returning an incorrect value in the first place (for example returning 'false' which == NS_OK when we meant to indicate failure)

Comment on attachment 650096 [details] [diff] [review] Patch part 3 -- Fix nsresult |= Review of attachment 650096 [details] [diff] [review]: ----------------------------------------------------------------- I'm OK with this patch going ahead as is, or with the changes I suggest; r+ either way. ::: mailnews/base/src/nsMsgContentPolicy.cpp @@ +335,4 @@ > > NS_ENSURE_SUCCESS(rv, false); > + NS_ENSURE_SUCCESS(rv2, false); > + NS_ENSURE_SUCCESS(rv3, false); I think we can take early exits on these instead of having extra variables: nsresult rv = aRequestingLocation->SchemeIs("chrome", &isChrome); NS_ENSURE_SUCCESS(rv, false); rv = aRequestingLocation->SchemeIs("resource", &isRes); NS_ENSURE_SUCCESS(rv, false); rv = aRequestingLocation->SchemeIs("file", &isFile); NS_ENSURE_SUCCESS(rv, false); @@ +389,4 @@ > > NS_ENSURE_SUCCESS(rv, false); > + NS_ENSURE_SUCCESS(rv2, false); > + NS_ENSURE_SUCCESS(rv3, false); Early exits again: nsresult rv = aRequestingLocation->SchemeIs("chrome", &isChrome); NS_ENSURE_SUCCESS(rv, false); rv = aRequestingLocation->SchemeIs("resource", &isRes); NS_ENSURE_SUCCESS(rv, false); rv = aRequestingLocation->SchemeIs("data", &isData); NS_ENSURE_SUCCESS(rv, false); @@ +409,5 @@ > > // Error condition - we must return true so that we block. > NS_ENSURE_SUCCESS(rv, true); > + NS_ENSURE_SUCCESS(rv2, true); > + NS_ENSURE_SUCCESS(rv3, true); And again: nsresult rv = aRequestingLocation->SchemeIs("http", &isHttp); NS_ENSURE_SUCCESS(rv, false); rv = aRequestingLocation->SchemeIs("https", &isHttps); NS_ENSURE_SUCCESS(rv, false); rv = aRequestingLocation->SchemeIs("file", &isFile); NS_ENSURE_SUCCESS(rv, false);

Comment on attachment 650098 [details] [diff] [review] Patch part 4 -- Change types to nsresult Review of attachment 650098 [details] [diff] [review]: ----------------------------------------------------------------- Standard8 can overrule me on these nits if he wants; otherwise, I'd like to give it a quick re-review after the patch is updated. ::: mailnews/base/util/nsMsgLineBuffer.cpp @@ +93,5 @@ > /* The last buffer ended with a CR. The new buffer does not start > with a LF. This old buffer should be shipped out and discarded. */ > PR_ASSERT(m_bufferSize > m_bufferPos); > + // XXX -1 is not a valid nsresult > + if (m_bufferSize <= m_bufferPos) return static_cast<nsresult>(-1); The only callers that don't ignore the result treat it as an nsresult (aside from casting it back and forth to int), so let's use valid nsresult codes here: if (m_bufferSize <= m_bufferPos) return NS_ERROR_UNEXPECTED; @@ +101,1 @@ > return status; Similarly, if (ConvertAndSendBuffer() == -1) return NS_ERROR_FAILURE; @@ +170,5 @@ > + return NS_OK; > + > + // XXX This returns -1 on error, not an nsresult > + status = static_cast<nsresult>(ConvertAndSendBuffer()); > + if (NS_FAILED(status)) return status; if (ConvertAndSendBuffer() == -1) return NS_ERROR_FAILURE; ::: mailnews/compose/src/nsSmtpProtocol.cpp @@ +410,1 @@ > return rv; I'd prefer to leave AuthLoginResponse() returning PRInt32, so PRInt32 rv = AuthLoginReponse(nullptr, 0); if (rv < 0) return NS_ERROR_FAILURE; @@ +1054,5 @@ > } > > > > +nsresult nsSmtpProtocol::AuthLoginResponse(nsIInputStream * stream, PRUint32 length) All the other internal methods in this class return integers, not nsresult; I'd prefer to not change this one. ::: mailnews/compose/src/nsSmtpProtocol.h @@ +173,5 @@ > PRInt32 AuthLoginStep0(); > PRInt32 AuthLoginStep0Response(); > PRInt32 AuthLoginStep1(); > PRInt32 AuthLoginStep2(); > + nsresult AuthLoginResponse(nsIInputStream * stream, PRUint32 length); Leave this as PRInt32 to be consistent with the others ::: mailnews/local/src/nsParseMailbox.h @@ +155,5 @@ > > void SetDB (nsIMsgDatabase *mailDB) {m_mailDB = mailDB; } > > // message socket libnet callbacks, which come through folder pane > + nsresult ProcessMailboxInputStream(nsIURI* aURL, nsIInputStream *aIStream, PRUint32 aLength); Was the change from virtual to non-virtual deliberate?

(In reply to Irving Reid (:irving) from comment #29) > The only callers that don't ignore the result treat it as an nsresult (aside > from casting it back and forth to int), so let's use valid nsresult codes > here: > > if (m_bufferSize <= m_bufferPos) > return NS_ERROR_UNEXPECTED; How do you figure? It's called by nsMsgMailboxParser::ProcessMailboxInputStream, which just returns the result. That's called by nsMsgMailboxParser::OnDataAvailable, which just returns the result. That's a virtual XPCOM method with like a zillion callers. Do all of them really treat the result properly? I really don't want to make this kind of change if it might change behavior. Likewise for the other two comments on the file. > ::: mailnews/compose/src/nsSmtpProtocol.cpp > @@ +410,1 @@ > > return rv; > > I'd prefer to leave AuthLoginResponse() returning PRInt32, so > > PRInt32 rv = AuthLoginReponse(nullptr, 0); > if (rv < 0) > return NS_ERROR_FAILURE; > > @@ +1054,5 @@ > > } > > > > > > > > +nsresult nsSmtpProtocol::AuthLoginResponse(nsIInputStream * stream, PRUint32 length) > > All the other internal methods in this class return integers, not nsresult; > I'd prefer to not change this one. Instead, I changed all the others to return nsresult (or void, for the ones that always returned NS_OK). Currently, changing things that really return nsresult to return int works, because nsresult can still be implicitly converted to int. When nsresult is made an enum class rather than a regular enum, this will no longer be possible, so they'll have to be ported at that point -- unless you change every single "return NS_ERROR_FAILURE;" to "return static_cast<int>(NS_ERROR_FAILURE);". May as well do it now. > ::: mailnews/local/src/nsParseMailbox.h > @@ +155,5 @@ > > > > void SetDB (nsIMsgDatabase *mailDB) {m_mailDB = mailDB; } > > > > // message socket libnet callbacks, which come through folder pane > > + nsresult ProcessMailboxInputStream(nsIURI* aURL, nsIInputStream *aIStream, PRUint32 aLength); > > Was the change from virtual to non-virtual deliberate? Yes. If it were really used as a virtual function, I'd have to be a lot more careful in changing its return type, lest it mask a function it previously overrode. In fact, the method doesn't override anything nor is it overridden by anything, so I made it non-virtual while I was changing the signature anyway.

This makes it consistent again. The methods changed are all private, so I checked callers where necessary and don't think any of this changes behavior.

I don't know how this slipped through the cracks before. Possibly it's related to the institution of nullptr, but returning nsnull here should have been caught too, since numeric types don't implicitly convert to enum.

Comment on attachment 650098 [details] [diff] [review] Patch part 4 -- Change types to nsresult Re-requesting review per comment 30.

Comment on attachment 650098 [details] [diff] [review] Patch part 4 -- Change types to nsresult (In reply to :Aryeh Gregor from comment #30) > I really don't want to make this kind of change if it might change behavior. > Likewise for the other two comments on the file. OK, I'll move the change to a separate patch - I've been accumulating a patch containing all the places where I think our old return values were incorrect, and I'll put that up for separate review later. > Instead, I changed all the others to return nsresult (or void, for the ones > that always returned NS_OK). OK. > > Was the change from virtual to non-virtual deliberate? > > Yes. If it were really used as a virtual function, I'd have to be a lot > more careful in changing its return type, lest it mask a function it > previously overrode. In fact, the method doesn't override anything nor is > it overridden by anything, so I made it non-virtual while I was changing the > signature anyway. Great, thanks.

(In reply to :Aryeh Gregor from comment #30) (in the context of returning static_cast<nsresult>(-1) ) > How do you figure? It's called by > nsMsgMailboxParser::ProcessMailboxInputStream, which just returns the > result. That's called by nsMsgMailboxParser::OnDataAvailable, which just > returns the result. That's a virtual XPCOM method with like a zillion > callers. Do all of them really treat the result properly? > > I really don't want to make this kind of change if it might change behavior. > Likewise for the other two comments on the file. Just for my own understanding, the result of static_cast<nsresult>(-1) will show up as a failure when tested with the NS_FAILED() macro, right?

Correct. NS_FAILED just tests if the high bit is set when the argument is interpreted as PRUint32, and -1 has all bits set.

Comment on attachment 650103 [details] [diff] [review] Patch part 6 -- Change types away from nsresult Review of attachment 650103 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsMsgSend.cpp @@ +1050,5 @@ > PRInt32 > nsMsgComposeAndSend::PreProcessPart(nsMsgAttachmentHandler *ma, > nsMsgSendPart *toppart) // The very top most container of the message > { > + int status; This function still uses NS_FAILED(status) to test the return values of lots of calls - should we clean that up here or in a separate patch (or just leave it)? ::: mailnews/imap/src/nsImapProtocol.cpp @@ +4518,5 @@ > + if (request) { > + nsresult rv; > + request->GetStatus(&rv); > + returnValue = static_cast<bool>(rv); > + } Wow, the old code here was really ugly, almost certainly broken, and might be the cause of some long-standing bugs with not handling dead IMAP connections correctly; I'll investigate further and file a separate bug if necessary. That said, as long as we don't lose bits casting the nsresult to bool and back, this preserves the behaviour of the existing code.

(In reply to Irving Reid (:irving) from comment #37) > This function still uses NS_FAILED(status) to test the return values of lots > of calls - should we clean that up here or in a separate patch (or just > leave it)? NS_FAILED() is soon going to be an inline function that takes nsresult as its arg (bug 768865, which I hope to land today-ish). This means when we convert nsresult to an enum, all attempts to use it on non-nsresults will fail. So this will have to be cleaned up separately before the enum conversion lands. I think we can leave it for a later bug. > ::: mailnews/imap/src/nsImapProtocol.cpp > @@ +4518,5 @@ > > + if (request) { > > + nsresult rv; > > + request->GetStatus(&rv); > > + returnValue = static_cast<bool>(rv); > > + } > > Wow, the old code here was really ugly, almost certainly broken, and might > be the cause of some long-standing bugs with not handling dead IMAP > connections correctly; I'll investigate further and file a separate bug if > necessary. > > That said, as long as we don't lose bits casting the nsresult to bool and > back, this preserves the behaviour of the existing code. Oops -- this change is broken. The NS_SUCCEEDED() check just below now will return different results. Here's a better diff for that file: if (NS_SUCCEEDED(returnValue)) // check the other way of cancelling. { ReentrantMonitorAutoEnter threadDeathMon(m_threadDeathMonitor); - returnValue = m_threadShouldDie; - } - return returnValue; + // XXX Casting bool to nsresult + returnValue = static_cast<nsresult>(m_threadShouldDie); + } + // XXX Casting nsresult to bool + return static_cast<bool>(returnValue); } This should preserve existing behavior exactly, just making implicit conversions explicit. I'll attach a new patch in a sec.

Same as last patch except for the difference detailed in the previous comment.

Comment on attachment 652406 [details] [diff] [review] Patch part 6, v2 -- Change types away from nsresult Review of attachment 652406 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/imap/src/nsImapProtocol.cpp @@ +4524,5 @@ > + // XXX Casting bool to nsresult > + returnValue = static_cast<nsresult>(m_threadShouldDie); > + } > + // XXX Casting nsresult to bool > + return static_cast<bool>(returnValue); I like this.

https://hg.mozilla.org/comm-central/rev/9d106bbca8d9 https://hg.mozilla.org/comm-central/rev/256178ca5b41 https://hg.mozilla.org/comm-central/rev/9354cabcfa41 https://hg.mozilla.org/comm-central/rev/4e6feae89ab4 https://hg.mozilla.org/comm-central/rev/eb4e923be691 https://hg.mozilla.org/comm-central/rev/decafe2f7089 https://hg.mozilla.org/comm-central/rev/2657de7f561b