Last Comment Bug 824296 - fix some signed/unsigned comparison problems and compile warnings in nsMsgFilterList.cpp
: fix some signed/unsigned comparison problems and compile warnings in nsMsgFil...
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
-- trivial (vote)
: Thunderbird 20.0
Assigned To: :aceman
Depends on: 821743
  Show dependency treegraph
Reported: 2012-12-22 14:15 PST by :aceman
Modified: 2012-12-30 16:36 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (3.29 KB, patch)
2012-12-22 14:47 PST, :aceman
neil: review+
Details | Diff | Splinter Review
patch v2 (3.23 KB, patch)
2012-12-25 13:02 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description User image :aceman 2012-12-22 14:15:20 PST
mailnews/base/search/src/nsMsgFilterList.cpp: In member function 'nsresult nsMsgFilterList::SaveTextFilters(nsIOutputStream*)':
mailnews/base/search/src/nsMsgFilterList.cpp:871:15: warning: variable 'attribStr' set but not used [-Wunused-but-set-variable]
mailnews/base/search/src/nsMsgFilterList.cpp: In member function 'virtual nsresult nsMsgFilterList::MoveFilter(nsIMsgFilter*, nsMsgFilterMotionValue)':
mailnews/base/search/src/nsMsgFilterList.cpp:1043:60: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

From bug 821743: 2012-12-22 21:51:03 CET

Comment on attachment 695244 [details] [diff] [review] [diff] [review]

>+  NS_ENSURE_ARG_MAX(aIndex, m_actionList.Length() - 1);
Not sure that this does the right thing for an empty list.

The same problem happens here in nsIMsgFilterList.cpp.
Comment 1 User image :aceman 2012-12-22 14:47:01 PST
Created attachment 695261 [details] [diff] [review]
Comment 2 User image 2012-12-24 14:21:58 PST
Comment on attachment 695261 [details] [diff] [review]

> nsresult nsMsgFilterList::GetFilterAt(uint32_t filterIndex, nsIMsgFilter **filter)
> {
>-  uint32_t filterCount = 0;
>-  GetFilterCount(&filterCount);
>-  NS_ENSURE_ARG_MAX(filterIndex, filterCount - 1);
>+  NS_ENSURE_ARG(filterIndex < m_filters.Length());
You're trying to fix too many bugs at once and ending up with a mishmash. Although it's a good idea to replace calls to GetFilterCount with direct access to Length() you should file a separate bugs and fix up all the cases at once. Please put this back for now so that it looks like the check in MoveFilterAt below. r=me with that fixed.
Comment 3 User image :aceman 2012-12-25 05:56:44 PST
I do not want to replace any other occurrence of GetFilterCount in that file. If you see the file history I am the one who put them there originally ;) I just wanted to replace this single one. So I will not file any separate bug for that.

So if you consider the change to be good as you say, either accept it here, or I take it out as you wish, but then it will not be done later (at least not by me).
Comment 4 User image 2012-12-25 12:33:01 PST
When I said "calls" I meant "all the internal calls". I didn't mean "one call because I felt like it". I can't see any justification for only changing one call.
Comment 5 User image :aceman 2012-12-25 13:02:43 PST
Created attachment 695659 [details] [diff] [review]
patch v2

As you wish.
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2012-12-30 16:36:26 PST

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