Last Comment Bug 715938 - Replace last 2 references to nsCRT::strlen(), in Imap code
: Replace last 2 references to nsCRT::strlen(), in Imap code
Status: VERIFIED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 13.0
Assigned To: Shriram (irc: Mavericks) Away
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 150073
  Show dependency treegraph
 
Reported: 2012-01-06 10:02 PST by Serge Gautherie (:sgautherie)
Modified: 2012-02-26 23:15 PST (History)
3 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch that removes nsCRT:: in two matching lines as per URL field in bug (1.98 KB, patch)
2012-02-18 23:19 PST, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch that adds spaces at two matching lines as per URL field in bug [= Diff over previous patch only] (1.98 KB, patch)
2012-02-19 04:46 PST, Shriram (irc: Mavericks) Away
mozilla: review-
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch that fixes formatting mentioned in last comment [= Diff over previous patch only] (1.86 KB, patch)
2012-02-22 11:52 PST, Shriram (irc: Mavericks) Away
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch that includes all relevant changes made as per comments (1.87 KB, patch)
2012-02-25 01:06 PST, Shriram (irc: Mavericks) Away
mozilla: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch that adds an explicit statement as per last comment (1.93 KB, patch)
2012-02-25 18:44 PST, Shriram (irc: Mavericks) Away
no flags Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-01-06 10:02:13 PST
These references are in comments.

Simply remove 'nsCRT::' part.
Comment 1 Shriram (irc: Mavericks) Away 2012-02-18 02:40:53 PST
(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) 2012-02-18 13:28:05 PST
(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) Away 2012-02-18 19:17:15 PST
(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) 2012-02-18 19:22:06 PST
(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) Away 2012-02-18 20:24:31 PST
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) Away 2012-02-18 20:26:29 PST
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) 2012-02-18 20:36:40 PST
Well, did you miss comment 0?
Comment 8 Shriram (irc: Mavericks) Away 2012-02-18 21:52:13 PST
(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) Away 2012-02-18 23:19:32 PST
Created attachment 598632 [details] [diff] [review]
Patch that removes nsCRT:: in two matching lines as per URL field in bug
Comment 10 Serge Gautherie (:sgautherie) 2012-02-19 00:58:32 PST
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) Away 2012-02-19 04:46:41 PST
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]
Comment 12 David :Bienvenu 2012-02-21 08:01:07 PST
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) Away 2012-02-22 11:52:54 PST
Created attachment 599719 [details] [diff] [review]
Patch that fixes formatting mentioned in last comment
[= Diff over previous patch only]
Comment 14 Shriram (irc: Mavericks) Away 2012-02-22 11:53:06 PST
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) 2012-02-22 22:13:27 PST
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) Away 2012-02-25 01:06:30 PST
Created attachment 600633 [details] [diff] [review]
Patch that includes all relevant changes made as per comments
Comment 17 David :Bienvenu 2012-02-25 06:51:48 PST
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) 2012-02-25 07:14:59 PST
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) Away 2012-02-25 09:13:15 PST
(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) 2012-02-25 10:10:12 PST
(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) Away 2012-02-25 17:05:41 PST
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) Away 2012-02-25 18:44:22 PST
Created attachment 600738 [details] [diff] [review]
Patch that adds an explicit statement as per last comment
Comment 23 Serge Gautherie (:sgautherie) 2012-02-25 23:47:18 PST
(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) Away 2012-02-26 02:29:10 PST
> 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] 2012-02-26 12:33:19 PST
http://hg.mozilla.org/comm-central/rev/3a940f4e6279
Comment 26 Serge Gautherie (:sgautherie) 2012-02-26 23:15:51 PST
V.Fixed, per MXR.

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