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

VERIFIED FIXED in Thunderbird 13.0

Status

MailNews Core
Networking: IMAP
--
trivial
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: Shriram (irc: Mavericks))

Tracking

Trunk
Thunderbird 13.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
These references are in comments.

Simply remove 'nsCRT::' part.
Flags: in-testsuite-
(Assignee)

Comment 1

5 years ago
(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
(Reporter)

Comment 2

5 years ago
(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...
(Assignee)

Comment 3

5 years ago
(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
(Reporter)

Comment 4

5 years ago
(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...
(Assignee)

Comment 5

5 years ago
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?
(Assignee)

Comment 6

5 years ago
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.
(Reporter)

Comment 7

5 years ago
Well, did you miss comment 0?
(Assignee)

Comment 8

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

I can't believe I missed it. Sheesh!
(Assignee)

Comment 9

5 years ago
Created attachment 598632 [details] [diff] [review]
Patch that removes nsCRT:: in two matching lines as per URL field in bug
(Reporter)

Comment 10

5 years ago
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 '//'.
Attachment #598632 - Flags: review?(dbienvenu)
Attachment #598632 - Flags: feedback+
(Reporter)

Updated

5 years ago
Assignee: nobody → kshriram18
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 13.0
(Assignee)

Comment 11

5 years ago
Created attachment 598652 [details] [diff] [review]
Patch that adds spaces at two matching lines as per URL field in bug
[= Diff over previous patch only]
(Reporter)

Updated

5 years ago
Attachment #598652 - Flags: review?(dbienvenu)
Attachment #598652 - Flags: feedback+
(Reporter)

Updated

5 years ago
Attachment #598632 - Attachment is obsolete: true
Attachment #598632 - Flags: review?(dbienvenu)

Comment 12

5 years ago
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);
Attachment #598652 - Flags: review?(dbienvenu) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 599719 [details] [diff] [review]
Patch that fixes formatting mentioned in last comment
[= Diff over previous patch only]
Attachment #598652 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
It replaces nsCRT as per title of the bug including formatting.
Latest patch takes care of formatting issue in your comment too.
(Reporter)

Comment 15

5 years ago
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.
Attachment #599719 - Attachment is obsolete: true
Attachment #599719 - Flags: feedback+
(Assignee)

Comment 16

5 years ago
Created attachment 600633 [details] [diff] [review]
Patch that includes all relevant changes made as per comments
(Reporter)

Updated

5 years ago
Attachment #599719 - Attachment description: Patch that fixes formatting mentioned in last comment → Patch that fixes formatting mentioned in last comment [= Diff over previous patch only]
(Reporter)

Updated

5 years ago
Attachment #598652 - Attachment description: Patch that adds spaces at two matching lines as per URL field in bug → Patch that adds spaces at two matching lines as per URL field in bug [= Diff over previous patch only]
(Reporter)

Updated

5 years ago
Attachment #600633 - Flags: review?(dbienvenu)
Attachment #600633 - Flags: feedback+

Comment 17

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

looks fine, thx.
Attachment #600633 - Flags: review?(dbienvenu) → review+
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [good first bug]
(Reporter)

Comment 18

5 years ago
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
(Assignee)

Comment 19

5 years ago
(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?
(Reporter)

Comment 20

5 years ago
(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."
(Assignee)

Comment 21

5 years ago
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?
(Assignee)

Comment 22

5 years ago
Created attachment 600738 [details] [diff] [review]
Patch that adds an explicit statement as per last comment
Attachment #600633 - Attachment is obsolete: true
(Reporter)

Comment 23

5 years ago
(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).
(Assignee)

Comment 24

5 years ago
> 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.
http://hg.mozilla.org/comm-central/rev/3a940f4e6279
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 26

5 years ago
V.Fixed, per MXR.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.