Closed
Bug 870236
Opened 11 years ago
Closed 11 years ago
Consolidate return value behaviour of nsMsgSearchTerm::Match* functions
Categories
(MailNews Core :: Search, defect)
MailNews Core
Search
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 24.0
People
(Reporter: aceman, Assigned: sshagarwal)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][mentor=aceman][lang=c++])
Attachments
(1 file, 8 obsolete files)
27.94 KB,
patch
|
rkent
:
review+
aceman
:
feedback+
|
Details | Diff | Splinter Review |
There are some inconsistencies in the nsMsgSearchTerm::Match* functions in what they return in case of failure: - only some of the functions return NS_ERROR_FAILURE in case an invalid nsMsgSearchOp was used. - some of them call NS_ASSERTION in that case, some do NS_ERROR, some do nothing. I propose to make all call NS_ERROR. - some even return true in *pResult (a match) if invalid comparison operation was used (see e.g. http://hg.mozilla.org/comm-central/file/4c366a1789a7/mailnews/base/search/src/nsMsgSearchTerm.cpp#l1473) There is also some more cleanup possible: -'break' in the 'default:' branch of 'switch'. -'Else' after return. -Change 'err' to 'rv' for coding style. -empty line after NS_ENSURE_ARG_POINTER() everywhere. -merge: "nsresult rv = MatchString(dbHdrValue, nullptr, aResult); return rv;" -comment that MatchStatus() not used only for nsMsgMessageFlags but also for nsMsgFolderFlags (as they are both 'unsigned long' for now). And that in this function nsMsgSearchOp::Is and nsMsgSearchOp::Isnt is intentionally used as Contains/DoesntContain for legacy reasons. But first I'd like confirmation from rkent that all these changes are welcome.
Comment 1•11 years ago
|
||
The cleanups that you mention all make sense (I assume "-'Else' after return." means to remove Else after return). I also like seeing them in a cleanup bug rather than complicating the review of a fault bug.
(In reply to Kent James (:rkent) from comment #1) > The cleanups that you mention all make sense (I assume "-'Else' after > return." means to remove Else after return). Yes. The same as removing 'break' in the 'default:' branch of 'switch'. > I also like seeing them in a > cleanup bug rather than complicating the review of a fault bug. Thanks.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #749919 -
Flags: feedback?(acelists)
Assignee | ||
Comment 4•11 years ago
|
||
Sir, I have tried to make the changes. Please let me know if I am moving in the right direction.
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 749919 [details] [diff] [review] Patch I missed a few things in this. Sorry.
Attachment #749919 -
Attachment is obsolete: true
Attachment #749919 -
Flags: feedback?(acelists)
Assignee | ||
Comment 6•11 years ago
|
||
Sir, Sorry for the last patch. I have made the necessary changes, please let me know what needs to be changed in this.
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Attachment #749936 -
Flags: feedback?(acelists)
Comment on attachment 749936 [details] [diff] [review] Patch Review of attachment 749936 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/search/src/nsMsgSearchTerm.cpp @@ +851,3 @@ > nsCString dbHdrValue; > aHdr->GetStringProperty(m_hdrProperty.get(), getter_Copies(dbHdrValue)); > + return MatchString(dbHdrValue, nullptr, aResult); Good. @@ -870,5 @@ > NS_IMETHODIMP nsMsgSearchTerm::MatchUint32HdrProperty(nsIMsgDBHdr *aHdr, bool *aResult) > { > NS_ENSURE_ARG_POINTER(aResult); > NS_ENSURE_ARG_POINTER(aHdr); > - Sorry, the bug description was meant to keep and add blank line after NS_ENSURE_ARG_POINTER block where missing. Not remove it. @@ -894,5 @@ > if (dbHdrValue != m_value.u.msgStatus) > result = true; > break; > - default: > - break; Do not remove this, add rv=NS_ERROR_FAILURE and NS_ERROR("invalid compare op for uint") @@ -1065,1 @@ > } This was meant to be if () return x return y @@ -1079,5 @@ > // Save some performance for opIsEmpty / opIsntEmpty > if(nsMsgSearchOp::IsEmpty != m_operator && nsMsgSearchOp::IsntEmpty != m_operator) > { > - NS_ASSERTION(MsgIsUTF8(nsDependentCString(m_value.string)), > - "m_value.string is not UTF-8"); You removed the condition. Revert this back. The change of NS_ASSERTION to NS_ERROR was only meant in the default branch of switch on nsMsgSearchOp . @@ -1087,5 @@ > { > ConvertToUnicode(charset, nsCString(stringToMatch), utf16StrToMatch); > } > else { > - NS_ASSERTION(MsgIsUTF8(stringToMatch), "stringToMatch is not UTF-8"); Revert. @@ +1122,5 @@ > nsCaseInsensitiveStringComparator())) > result = true; > break; > default: > + NS_ERROR("invalid operator matching search results"); This is good, bud add rv=NS_ERROR_FAILURE and make sure result = false here. @@ +1141,5 @@ > NS_ENSURE_ARG_POINTER(pResult); > *pResult = false; > bool result; > + nsresult rv = InitHeaderAddressParser(); > + NS_ENSURE_SUCCESS(rv, rv); Good. @@ -1173,5 @@ > > if (NS_SUCCEEDED(parseErr) && count > 0) > { > - NS_ASSERTION(names, "couldn't get names"); > - NS_ASSERTION(addresses, "couldn't get addresses"); Revert. @@ +1244,5 @@ > result = true; > break; > default: > rv = NS_ERROR_FAILURE; > + NS_ERROR("invalid compare op for dates"); Good. @@ +1290,5 @@ > result = true; > } > break; > default: > + NS_ERROR("invalid compare op for msg age"); rv = failure and ensure result = false. @@ -1339,5 @@ > if (sizeToMatchKB == m_value.u.size) > result = true; > break; > - default: > - break; Add back and add rv=failure and NS_ERROR @@ -1435,5 @@ > if (aJunkPercent == m_value.u.junkPercent) > result = true; > break; > - default: > - break; Add back and add rv=failure and NS_ERROR @@ -1455,5 @@ > break; > default: > if (m_value.u.label != aLabelValue) > result = true; > - break; Good. But I think this should be converted to case nsMsgSearchOp::Isnt and add default: branch the standard rv= failure and NS_ERROR clauses. @@ +1987,2 @@ > } > + return rv; Changing functions other than Match* may be out of scope of this bug, so please do not change any other logic in them than 'rv' variable name.
Assignee | ||
Comment 8•11 years ago
|
||
Sir, Thanks for your feedback. I have made the changes you suggested.
Attachment #749936 -
Attachment is obsolete: true
Attachment #749936 -
Flags: feedback?(acelists)
Attachment #750349 -
Flags: feedback?(acelists)
Comment on attachment 750349 [details] [diff] [review] Patch v2 Review of attachment 750349 [details] [diff] [review]: ----------------------------------------------------------------- Much better now. Can you add the last wish from the bug description? -comment that MatchStatus() not used only for nsMsgMessageFlags but also for nsMsgFolderFlags (as they are both 'unsigned long' for now). And that in this function nsMsgSearchOp::Is and nsMsgSearchOp::Isnt is intentionally used as Contains/DoesntContain for legacy reasons. ::: mailnews/base/search/src/nsMsgSearchTerm.cpp @@ +872,5 @@ > { > NS_ENSURE_ARG_POINTER(aResult); > NS_ENSURE_ARG_POINTER(aHdr); > + > + nsresult rv = NS_OK; Good. @@ +1403,3 @@ > } > > *pResult = matches; So what is the value of 'matches' in case we got the operator failure? Ensure we set *pResult to false here. (In case the caller does not check the function return value (rv) for failure.) So probably just set 'matches' to false. I mentioned this in the bug description: - some even return true in *pResult (a match) if invalid comparison operation was used (see e.g. http://hg.mozilla.org/comm-central/file/4c366a1789a7/mailnews/base/search/src/nsMsgSearchTerm.cpp#l1473) @@ +1427,4 @@ > } > > *pResult = matches; > return rv; Again, is *pResult false in case of failure? And I think you need to also fix this problem in nsMsgSearchTerm::MatchStatus @@ +1476,3 @@ > if (m_value.u.label != aLabelValue) > result = true; > + default: This is what I wanted, but we need to make sure what the "Label" actually is and if what are the allowed operators in the UI. We will need rkent's advice here.
Assignee | ||
Comment 10•11 years ago
|
||
Sir, Thanks for the review, I have made the changes. For line #1476, default value of matches was false (at the time of its definition), so I think, if the control enters the default clause, it doesn't get changed anywhere and matches will be false. So, I haven't reassigned false to it in the default block. If this needs to be changed, please let me know.
Attachment #750349 -
Attachment is obsolete: true
Attachment #750349 -
Flags: feedback?(acelists)
Attachment #750473 -
Flags: feedback?(acelists)
Assignee | ||
Comment 11•11 years ago
|
||
Sir, I have made the changes you suggested. Please review this patch and suggest me if this is okay or what needs to be changed.
Attachment #750473 -
Attachment is obsolete: true
Attachment #750473 -
Flags: feedback?(acelists)
Attachment #750670 -
Flags: feedback?(acelists)
Reporter | ||
Comment 12•11 years ago
|
||
Comment on attachment 750670 [details] [diff] [review] Patch v4 Review of attachment 750670 [details] [diff] [review]: ----------------------------------------------------------------- Great, this is almost done now. Just small nits. ::: mailnews/base/search/src/nsMsgSearchTerm.cpp @@ +1067,5 @@ > > nsCOMPtr<nsIMimeConverter> mimeConverter = do_GetService(NS_MIME_CONVERTER_CONTRACTID); > nsAutoCString stringToMatch; > nsresult rv = mimeConverter->DecodeMimeHeaderToUTF8( > rfc2047string, charset, charsetOverride, false, stringToMatch); I now noticed we check this rv nowhere. Please set *pResult = false before this and add NS_ENSURE_SUCCESS(rv, rv) after this. @@ +1166,5 @@ > +nsresult nsMsgSearchTerm::MatchRfc822String (const char *string, > + const char *charset, > + bool charsetOverride, > + bool *pResult) > +{ This is good. @@ +1167,5 @@ > + const char *charset, > + bool charsetOverride, > + bool *pResult) > +{ > + NS_ENSURE_ARG_POINTER(pResult); Add blank line after this. @@ +1174,5 @@ > + nsresult rv = InitHeaderAddressParser(); > + NS_ENSURE_SUCCESS(rv, rv); > + // Isolate the RFC 822 parsing weirdnesses here. MSG_ParseRFC822Addresses > + // returns a catenated string of null-terminated strings, which we walk > + // across, tring to match the target string to either the name OR the address Have you reindented the whole function here? I don't think rkent will like that.
Attachment #750670 -
Flags: feedback?(acelists) → feedback+
Assignee | ||
Comment 13•11 years ago
|
||
Sir, I have made the changes you suggested. There was a leading extra space in every line of this function, so I removed that extra space from every line. If this isn't accepted, please let me know, I will revert it back.
Attachment #750670 -
Attachment is obsolete: true
Attachment #750704 -
Flags: review?(kent)
Attachment #750704 -
Flags: feedback?(acelists)
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 750704 [details] [diff] [review] Patch v4 (revised) Great!
Attachment #750704 -
Flags: feedback?(acelists) → feedback+
Comment 15•11 years ago
|
||
Comment on attachment 750704 [details] [diff] [review] Patch v4 (revised) Review of attachment 750704 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the slow review, I've been buried in my own user's issues for awhile. There are enough changes in this that even though the changes are all small, I should probably see it again before we finish it. I should also note that there is some small risk of regression here, as we are changing behavior in several locations so that invalid ops are now returning errors. I don;t think that will be an issue though. ::: mailnews/base/search/src/nsMsgSearchTerm.cpp @@ +740,5 @@ > bool ForFiltering, > bool *pResult) > { > NS_ENSURE_ARG_POINTER(pResult); > + Nit: no space at the beginning of this line. @@ +847,5 @@ > NS_IMETHODIMP nsMsgSearchTerm::MatchHdrProperty(nsIMsgDBHdr *aHdr, bool *aResult) > { > NS_ENSURE_ARG_POINTER(aResult); > NS_ENSURE_ARG_POINTER(aHdr); > + Nit: no space @@ +857,5 @@ > NS_IMETHODIMP nsMsgSearchTerm::MatchFolderFlag(nsIMsgDBHdr *aMsgToMatch, bool *aResult) > { > NS_ENSURE_ARG_POINTER(aMsgToMatch); > NS_ENSURE_ARG_POINTER(aResult); > + Nit: no space @@ +870,5 @@ > NS_IMETHODIMP nsMsgSearchTerm::MatchUint32HdrProperty(nsIMsgDBHdr *aHdr, bool *aResult) > { > NS_ENSURE_ARG_POINTER(aResult); > NS_ENSURE_ARG_POINTER(aHdr); > + Nit: no space @@ +896,5 @@ > result = true; > break; > default: > + rv = NS_ERROR_FAILURE; > + result = false; result defaults to false, no need to set it a second time. @@ +1039,5 @@ > + switch(m_operator) > + { > + case nsMsgSearchOp::IsInAB: > + if (cardForAddress) > + *pResult = true; Nit: indentation off @@ +1048,5 @@ > + break; > + default: > + rv = NS_ERROR_FAILURE; > + *pResult = false; > + NS_ERROR ("invalid compare op for address book"); Don't set *pResult = false, it defaults to false. Nit: space after ERROR. @@ +1054,1 @@ > NS_IF_RELEASE(cardForAddress); Since you are doing cleanup, could you redefine cardForAddress as nsCOMPtr<nsIAbCard> and eliminate this NS_IF_RELEASE? @@ +1066,5 @@ > NS_ENSURE_ARG_POINTER(pResult); > > nsCOMPtr<nsIMimeConverter> mimeConverter = do_GetService(NS_MIME_CONVERTER_CONTRACTID); > nsAutoCString stringToMatch; > + *pResult = false; This is not needed, the called routines should handle. @@ +1085,5 @@ > const char *charset, > bool *pResult) > { > NS_ENSURE_ARG_POINTER(pResult); > + Nit: extra space @@ +1149,5 @@ > result = true; > break; > default: > + rv = NS_ERROR_FAILURE; > + result = false; Do not set result, it defaults to false. @@ +1323,5 @@ > } > break; > default: > + rv = NS_ERROR_FAILURE; > + result = false; do not set result, it defaults to false. @@ +1363,5 @@ > result = true; > break; > default: > + rv = NS_ERROR_FAILURE; > + result = false; Do not set, this is the default @@ +1490,5 @@ > if (m_value.u.label != aLabelValue) > result = true; > + default: > + rv = NS_ERROR_FAILURE; > + result = false; Do not set, default value @@ +1693,5 @@ > if (p1 != p2) > result = true; > break; > default: > result = false; For consistency, remove this setting to default value
Attachment #750704 -
Flags: review?(kent) → review-
Reporter | ||
Comment 16•11 years ago
|
||
(In reply to Kent James (:rkent) from comment #15) >I should also note that there is some small risk of regression here, as we >are changing behavior in several locations so that invalid ops are now >returning errors. I don;t think that will be an issue though. That is the point here, we want to catch invalid ops instead of producing random results. > @@ +1066,5 @@ > > NS_ENSURE_ARG_POINTER(pResult); > > > > nsCOMPtr<nsIMimeConverter> mimeConverter = do_GetService(NS_MIME_CONVERTER_CONTRACTID); > > nsAutoCString stringToMatch; > > + *pResult = false; > > This is not needed, the called routines should handle. This is for the case where we return on NS_ENSURE_SUCCESS(rv, rv); In that case no other routine is called that would set the result. All other nits look fine, thanks for catching them.
Assignee | ||
Comment 17•11 years ago
|
||
I have made the changes proposed.
Attachment #750704 -
Attachment is obsolete: true
Attachment #755820 -
Flags: review?(kent)
Assignee | ||
Updated•11 years ago
|
Attachment #755820 -
Flags: feedback?(acelists)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #755820 -
Attachment is obsolete: true
Attachment #755820 -
Flags: review?(kent)
Attachment #755820 -
Flags: feedback?(acelists)
Attachment #755822 -
Flags: review?(kent)
Attachment #755822 -
Flags: feedback?(acelists)
Reporter | ||
Comment 19•11 years ago
|
||
Comment on attachment 755822 [details] [diff] [review] Patch v5 Review of attachment 755822 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/search/src/nsMsgSearchTerm.cpp @@ +1030,5 @@ > return rv; > > if (mDirectory) > { > + nsCOMPtr<nsIAbCard> cardForAddress = nullptr; Not sure we need the nullptr assignment here. But rkent will know better. @@ +1270,5 @@ > result = true; > break; > default: > rv = NS_ERROR_FAILURE; > + result = false; Not needed, result is false at the start of the function. @@ +1459,5 @@ > result = true; > break; > default: > + rv = NS_ERROR_FAILURE; > + result = false; Not needed, result is false at the start of the function. @@ +1511,5 @@ > matches = !matches; > break; > default: > rv = NS_ERROR_FAILURE; > + matches = false; Good, this is needed.
Attachment #755822 -
Flags: feedback?(acelists) → feedback+
Assignee | ||
Comment 20•11 years ago
|
||
Sir, I have made the changes proposed. But I am still not sure about removing *pResult = false near line#1066.
Attachment #755822 -
Attachment is obsolete: true
Attachment #755822 -
Flags: review?(kent)
Attachment #756021 -
Flags: review?(kent)
Attachment #756021 -
Flags: feedback?(acelists)
Reporter | ||
Comment 21•11 years ago
|
||
Comment on attachment 756021 [details] [diff] [review] Patch v5 (revised) Review of attachment 756021 [details] [diff] [review]: ----------------------------------------------------------------- I would leave "*pResult = false near line#1066" in, but rkent must decide.
Attachment #756021 -
Flags: feedback?(acelists) → feedback+
Comment 22•11 years ago
|
||
Comment on attachment 756021 [details] [diff] [review] Patch v5 (revised) Review of attachment 756021 [details] [diff] [review]: ----------------------------------------------------------------- I suppose I could add a few more nits, but I'm not going to. So let's just call this done.
Attachment #756021 -
Flags: review?(kent) → review+
Assignee | ||
Comment 23•11 years ago
|
||
If there are a few changes to suggest, I don't know why you are not suggesting them.
Keywords: checkin-needed
Comment 24•11 years ago
|
||
(In reply to Suyash Agarwal (:sshagarwal) from comment #23) > If there are a few changes to suggest, I don't know why you are not > suggesting them. Because I think that we expend too much effort on nits as it is.
Comment 25•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/25027c73dd53
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
You need to log in
before you can comment on or make changes to this bug.
Description
•