610611 - redundant GetMessageKey() call

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Boying Lu]

The GetMessageKey() call is redundant and should be removed.

Comment 1

[Boying Lu]

Sorry, for clicking submit button too quickly.
I mean the call at http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgThread.cpp#209 is redundant.

Comment 2

[Boying Lu]

Attachment: patch

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 489124 [details] [diff] [review]
patch

>   nsCOMPtr <nsIMsgDBHdr> ancestorHdr = newParentOfOldRoot;
>   nsMsgKey newRoot;
>-  newParentOfOldRoot->GetMessageKey(&newRoot);
>   mdb_pos outPos;
> 
>   nsMsgKey newHdrAncestor;
>   ancestorHdr->GetMessageKey(&newRoot);

This seems wrong, looking further below newHdrAncestor is not initialised and used uninitialised.

However, these newRoot and newHdrAncestor would then end up the same. That might be right, but then we could just assign rather than getting the message key twice.

Although it also could be that just getting the newHdrAncestor is redundant as you've got the patch. In which case I would tend to reorder the variables a bit.

So I'm not quite sure which shape this function is meant to be in. So I'm going to pass the review to David and let him have a look.

Comment 4

[David :Bienvenu]

newHdrAncestor is initialized here:
    ancestorHdr->GetThreadParent(&newHdrAncestor);

ThreadParent is not the same as MessageKey, so I don't think newHdrAncestor and newRoot are the same.

So this patch looks OK, other than perhaps moving the decl of newRoot to just before it's used.

Comment 5

[Boying Lu]

Attachment: move the decl of newRoot before its first reference

Comment 6

[David :Bienvenu]

Comment on attachment 489418 [details] [diff] [review]
move the decl of newRoot before its first reference

thx for the patch.

Comment 7

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/9a268939f29c