Last Comment Bug 798828 - fix "warning: overflow in implicit constant conversion [-Woverflow]" in mailnews/local/src/nsParseMailbox.cpp
: fix "warning: overflow in implicit constant conversion [-Woverflow]" in mail...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2012-10-06 15:41 PDT by :aceman
Modified: 2012-10-08 11:04 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (14.60 KB, patch)
2012-10-06 18:26 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v2 (14.53 KB, patch)
2012-10-07 07:11 PDT, :aceman
no flags Details | Diff | Splinter Review

Description :aceman 2012-10-06 15:41:11 PDT
I get these warnings with gcc 4.7 on linux:
mailnews/local/src/nsParseMailbox.cpp: In member function 'virtual int32_t nsMsgMailboxParser::HandleLine(char*, uint32_t)':
mailnews/local/src/nsParseMailbox.cpp:492:18: warning: overflow in implicit constant conversion [-Woverflow]
mailnews/local/src/nsParseMailbox.cpp: In member function 'int nsParseMailMessageState::FinalizeHeaders()':
mailnews/local/src/nsParseMailbox.cpp:1668:16: warning: overflow in implicit constant conversion [-Woverflow]

I convert the mentioned functions to nsresult which causes the need of other called functions to be converted to nsresult too.
Comment 1 :aceman 2012-10-06 18:26:40 PDT
Created attachment 668863 [details] [diff] [review]
patch
Comment 2 neil@parkwaycc.co.uk 2012-10-07 03:08:11 PDT
Comment on attachment 668863 [details] [diff] [review]
patch

>   if (line[0] == 'F' && IsEnvelopeLine(line, lineLength))
...
>     OnNewMessage(nullptr);
>-    status = StartNewEnvelope(line, lineLength);
>-    NS_ASSERTION(status >= 0, " error starting envelope parsing mailbox");
>+    rv = StartNewEnvelope(line, lineLength);
>+    NS_ASSERTION(NS_SUCCEEDED(rv), " error starting envelope parsing mailbox");
>     // at the start of each new message, update the progress bar
>     UpdateProgressPercent();
>-    if (status < 0)
>-      return status;
>+    NS_ENSURE_SUCCESS(rv, rv);
>   }
>   // otherwise, the message parser can handle it completely.
>-  else if (m_mailDB != nullptr)  // if no DB, do we need to parse at all?
>-    return ParseFolderLine(line, lineLength);
>-        else
>-          return NS_ERROR_NULL_POINTER; // need to error out if we don't have a db.
>-
>-  return 0;
>-
>+  else
>+  {
>+    if (m_mailDB != nullptr)  // if no DB, do we need to parse at all?
>+      return ParseFolderLine(line, lineLength);
>+    else
>+      return NS_ERROR_NULL_POINTER; // need to error out if we don't have a db.
>+  }
>+  return NS_OK;
This was badly written, yet your changes make it even worse. The original code looks like this:

if (line[0] == 'F' && IsEnvelopeLine(line, lineLength))
{
  ...
  status = StartNewEnvelope(line, lineLength);
  ...
  if (status < 0)
    return status;
}
else if (m_mailDB != nullptr)  // if no DB, do we need to parse at all?
  return ParseFolderLine(line, lineLength);
else
  return NS_ERROR_NULL_POINTER; // need to error out if we don't have a db.
return 0;

As you can see, the return 0 is only reached if the line is an envelope line, so you can move it into the if block, thus eliminating the inner condition. Having done this, the subsequent else clauses also become unnecessary since control can no longer reach there. The code therefore ends up something like this:

if (line[0] == 'F' && IsEnvelopeLine(line, lineLength))
{
  ...
  nsresult rv = StartNewEnvelope(line, lineLength);
  ...
  return rv;
}

if (m_mailDB != nullptr) // if no DB, do we need to parse at all?
  return ParseFolderLine(line, lineLength);

return NS_ERROR_NULL_POINTER; // need to error out if we don't have a db.

r=me with that fixed.
Comment 3 :aceman 2012-10-07 07:11:55 PDT
Created attachment 668905 [details] [diff] [review]
patch v2

Thanks Neil.
Comment 4 Mark Banner (:standard8) (afk until 26th July) 2012-10-08 00:23:13 PDT
Comment on attachment 668905 [details] [diff] [review]
patch v2

This doesn't change interfaces, so it doesn't need sr.
Comment 5 Mark Banner (:standard8) (afk until 26th July) 2012-10-08 11:04:51 PDT
Checked in: https://hg.mozilla.org/comm-central/rev/d1344e39bd90

Note You need to log in before you can comment on or make changes to this bug.