fix "warning: overflow in implicit constant conversion [-Woverflow]" in mailnews/local/src/nsParseMailbox.cpp

RESOLVED FIXED in Thunderbird 18.0

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 18.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

14.53 KB, patch
Details | Diff | Splinter Review
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.
Posted patch patch (obsolete) — Splinter Review
Attachment #668863 - Flags: superreview?(mbanner)
Attachment #668863 - Flags: review?(neil)
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.
Attachment #668863 - Flags: review?(neil) → review+
Posted patch patch v2Splinter Review
Thanks Neil.
Attachment #668863 - Attachment is obsolete: true
Attachment #668863 - Flags: superreview?(mbanner)
Attachment #668905 - Flags: superreview?(mbanner)
Comment on attachment 668905 [details] [diff] [review]
patch v2

This doesn't change interfaces, so it doesn't need sr.
Attachment #668905 - Flags: superreview?(mbanner)
Status: NEW → ASSIGNED
Keywords: checkin-needed
Checked in: https://hg.mozilla.org/comm-central/rev/d1344e39bd90
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.