424531 - warning: ‘attrib’ may be used uninitialized in nsMsgSearchTerm::GetTermAsString

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[Tuukka Tolvanen (sp3000)]

In member function ‘virtual nsresult nsMsgSearchTerm::GetTermAsString(nsACString_internal&)’:
http://mxr.mozilla.org/mozilla/source/mailnews/base/search/src/nsMsgSearchTerm.cpp#498
warning: ‘attrib’ may be used uninitialized in this function

assuming that
 511   if (m_attribute > nsMsgSearchAttrib::OtherHeader && m_attribute < nsMsgSearchAttrib::kNumMsgSearchAttributes)  // if arbitrary header, use it instead!
happens whenever
 507   ret = NS_MsgGetStringForAttribute(m_attribute, &attrib);
didn't set attrib, this is harmless.

It'd be clearer for the reader and the compiler if NS_MsgGetStringForAttribute wasn't uselessly called if its result isn't going to be used.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 2

[Tuukka Tolvanen (sp3000)]

turns out this won't shut up the warning -- the warning happens because for NS_MsgGetStringForAttribute ret == NS_OK is unconditional and does not imply that attrib was set. (NS_MsgGetStringForOperator behaves better with operatorStr and the compiler sees that as the implementation is in the same file, and correctly doesn't complain.) I guess the "if arbitrary header" check is coincidental with the case of attrib not being set if nobody's crashed here since 1999, but g++ certainly doesn't find that relation :\

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 312673 [details] [diff] [review]
proposed fix

r+sr=dmose

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Duh, I should have paid more attention to the compiler msgs...
With this patch the string is set to an empty string if the attrib isn't found, to make the compiler happy.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 312917 [details] [diff] [review]
proposed fix, v2

Fix the actual compiler warning too.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

r+sr=dmose.  I wouldn't be entirely surprised, though, if some compiler out there wants to generate a warning about direct assignment to a string like that.  |**string='\0'| might be safer.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 312917 [details] [diff] [review]
proposed fix, v2

+ing from the comment.

Comment 8

[Magnus Melin [:mkmelin]]

Fix checked in with the nit (one star though).

Checking in mailnews/base/search/src/nsMsgSearchTerm.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp,v  <--  nsMsgSearchTerm.cpp
new revision: 1.156; previous revision: 1.155
done