715938 - Replace last 2 references to nsCRT::strlen(), in Imap code

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Serge Gautherie (:sgautherie)]

These references are in comments.

Simply remove 'nsCRT::' part.

Comment 1

[Shriram (irc: Mavericks)]

(In reply to Serge Gautherie (:sgautherie) from comment #0)
> These references are in comments.
> 
> Simply remove 'nsCRT::' part.

As per lines from the URL, the nsCRT:: part's removed

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to Shriram (irc: Mavericks) from comment #1)
> As per lines from the URL, the nsCRT:: part's removed

I am not sure what you meant...

Comment 3

[Shriram (irc: Mavericks)]

(In reply to Serge Gautherie (:sgautherie) from comment #2)
> (In reply to Shriram (irc: Mavericks) from comment #1)
> > As per lines from the URL, the nsCRT:: part's removed
> 
> I am not sure what you meant...

AS per URL @ http://mxr.mozilla.org/comm-central/search?string=nsCRT%3A%3Astrlen&case=1&find=%2FnsImapServerResponseParser\.cpp%24

the two matching lines don't have nsCRT:: part anymore

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Shriram (irc: Mavericks) from comment #3)
> the two matching lines don't have nsCRT:: part anymore

Well, I still see them:
{
/mailnews/imap/src/nsImapServerResponseParser.cpp
    * line 2589 -- char *start = partNumber+5, *end = partNumber+5; // 5 == nsCRT::strlen("BODY[")
    * line 3136 -- //fCurrentTokenPlaceHolder = fLineOfTokens + nsCRT::strlen(fCurrentLine) - (charsReadSoFar - numberOfCharsInThisChunk);
}

If not, there would be no more matching lines...

Comment 5

[Shriram (irc: Mavericks)]

Oh, yeah I see them but they're commented, I  thought it's left in comments for some reason.
So, it needs to be removed as well. Would that be sufficient?

Comment 6

[Shriram (irc: Mavericks)]

Yes, my statement 'wo matching lines don't have nsCRT:: part anymore' is incorrect.

It should've been 'two matching lines have the nsCRT:: part commented' instead.

Comment 7

[Serge Gautherie (:sgautherie)]

Well, did you miss comment 0?

Comment 8

[Shriram (irc: Mavericks)]

(In reply to Serge Gautherie (:sgautherie) from comment #7)
> Well, did you miss comment 0?

I can't believe I missed it. Sheesh!

Comment 9

[Shriram (irc: Mavericks)]

Attachment: Patch that removes nsCRT:: in two matching lines as per URL field in bug

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 598632 [details] [diff] [review]
Patch that removes nsCRT:: in two matching lines as per URL field in bug

Review of attachment 598632 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +2585,5 @@
>  {
>    char *partNumber = PL_strdup(fNextToken);
>    if (partNumber)
>    {
> +    char *start = partNumber+5, *end = partNumber+5;	// 5 == strlen("BODY[")

While here, add a space around both '+'.

@@ +3132,5 @@
>      if (charsReadSoFar > numberOfCharsInThisChunk)
>      {
>        // move the lexical analyzer state to the end of this message because this message
>        // fetch ends in the middle of this line.
> +      //fCurrentTokenPlaceHolder = fLineOfTokens + strlen(fCurrentLine) - (charsReadSoFar - numberOfCharsInThisChunk);

While here, add a space after '//'.

Comment 11

[Shriram (irc: Mavericks)]

Attachment: Patch that adds spaces at two matching lines as per URL field in bug [= Diff over previous patch only]

Comment 12

[David :Bienvenu]

Comment on attachment 598652 [details] [diff] [review]
Patch that adds spaces at two matching lines as per URL field in bug
[= Diff over previous patch only]

thx for the patch - since you're only changing formatting, we might as well fix it all the way:

-    char *start = partNumber+5, *end = partNumber+5;	// 5 == strlen("BODY[")
+    char *start = partNumber + 5, *end = partNumber + 5;	// 5 == strlen("BODY[")

please remove all but one of the spaces before the //

and turn this into a two line comment, or better yet, just remove the comment entirely:

-      //fCurrentTokenPlaceHolder = fLineOfTokens + strlen(fCurrentLine) - (charsReadSoFar - numberOfCharsInThisChunk);
+      // fCurrentTokenPlaceHolder = fLineOfTokens + strlen(fCurrentLine) - (charsReadSoFar - numberOfCharsInThisChunk);

Comment 13

[Shriram (irc: Mavericks)]

Attachment: Patch that fixes formatting mentioned in last comment [= Diff over previous patch only]

Comment 14

[Shriram (irc: Mavericks)]

It replaces nsCRT as per title of the bug including formatting.
Latest patch takes care of formatting issue in your comment too.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 599719 [details] [diff] [review]
Patch that fixes formatting mentioned in last comment
[= Diff over previous patch only]

Good, but merge this diff with the previous patch.

Comment 16

[Shriram (irc: Mavericks)]

Attachment: Patch that includes all relevant changes made as per comments

Comment 17

[David :Bienvenu]

Comment on attachment 600633 [details] [diff] [review]
Patch that includes all relevant changes made as per comments

looks fine, thx.

Comment 18

[Serge Gautherie (:sgautherie)]

To ease c-n, can you attach a patch with an explicit comment, not "imported patch bug-715938-fix".
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 19

[Shriram (irc: Mavericks)]

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> To ease c-n, can you attach a patch with an explicit comment, not "imported
> patch bug-715938-fix".
> http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Would an explicit comment like 'Patch for bug 715938' suffice?

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to Shriram (irc: Mavericks) from comment #19)
> Would an explicit comment like 'Patch for bug 715938' suffice?

No, something more like "Bug 715938. Remove last 2 references to nsCRT::strlen(), in Imap code.".
To which you can add "f=sgautherie r=dbienvenu."

Comment 21

[Shriram (irc: Mavericks)]

As per 6.2 - add/change a message in http://mercurial.selenic.com/wiki/MqTutorial,
I did qrefresh -m "Bug 715938: contains all relevant changes made in comments" before producing the patch.
I saw message after -m in the summary when I did 'hg out'
I'm surprised "imported bug-715938-fix" wasn't replaced it.

I know that this is not what you wrote in last comment.
So, should I manually edit the patch and replace line with what you wrote?

Comment 22

[Shriram (irc: Mavericks)]

Attachment: Patch that adds an explicit statement as per last comment

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to Shriram (irc: Mavericks) from comment #22)
> Created attachment 600738 [details] [diff] [review]
> Patch that adds an explicit statement as per last comment

The included comment is perfect ;-)

Thanks for fixing this bug!

NB: In future bugs, what you write in attachment description should usually be in bug comment. And attachment description should instead tell what the patch is actually doing (more like the included comment).

Comment 24

[Shriram (irc: Mavericks)]

> NB: In future bugs, what you write in attachment description should usually
> be in bug comment. And attachment description should instead tell what the
> patch is actually doing (more like the included comment).

Thanks for the advice Serge. I shall keep that in mind.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/3a940f4e6279

Comment 26

[Serge Gautherie (:sgautherie)]

V.Fixed, per MXR.