Last Comment Bug 227633 - Replace 0x0a by nsCRT::LF, 0x0d by nsCRT::CR
: Replace 0x0a by nsCRT::LF, 0x0d by nsCRT::CR
Status: VERIFIED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: P4 trivial (vote)
: Thunderbird 13.0
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
Depends on:
Blocks: 728991
  Show dependency treegraph
 
Reported: 2003-12-06 05:37 PST by Serge Gautherie (:sgautherie)
Modified: 2012-02-20 18:34 PST (History)
4 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only) (21.21 KB, patch)
2003-12-06 07:58 PST, Serge Gautherie (:sgautherie)
dwitte: review+
Details | Diff | Splinter Review
(Av1a-Bw) <mailnews/*> (review only) (21.24 KB, patch)
2004-07-25 06:32 PDT, Serge Gautherie (:sgautherie)
dmose: superreview+
Details | Diff | Splinter Review
(Av1b) <mailnews/*> (full diff) (32.96 KB, patch)
2005-07-31 10:02 PDT, Serge Gautherie (:sgautherie)
benjamin: approval1.8b4-
Details | Diff | Splinter Review
(Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/ [Checked in: Comment 17] (37.65 KB, patch)
2012-01-07 08:43 PST, Serge Gautherie (:sgautherie)
standard8: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2003-12-06 05:37:40 PST
User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031030
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031030

Spun off from bug 119441 comment 25:
{
instead of 0x0A and 0x0D, consider using nsCRT::CR and nsCRT::LF, to make the
code more readable.
(if you want to fix the rest of the import code, that would be great, but that
can be spun off to a new bug.)
}


Reproducible: Always

Steps to Reproduce:
Comment 1 Serge Gautherie (:sgautherie) 2003-12-06 07:58:04 PST
Created attachment 136934 [details] [diff] [review]
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)

This does 1+ minor code optimizations too.

'review only':
Since all theses files are using Tabs,
the full patch has some more Tab->Space conversions,
only to better see the if/while/... blocks (involved in this patch).
Comment 2 Frank Wein [:mcsmurf] 2003-12-06 13:15:17 PST
Comment on attachment 136934 [details] [diff] [review]
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)

>-		else {
>+        else
> 			m_pBuffer[offset] = *pData;
>-		}
>+

Also i'm no expert at coding and C(++), but this looks wrong :)
Comment 3 dwitte@gmail.com 2004-07-21 17:54:27 PDT
Comment on attachment 136934 [details] [diff] [review]
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)

nice work! is there any more of this to come for the rest of the tree?

i'd suggest asking for rs= on this, rather than sr=... saves the superreviewer
from reading the patch.

r=dwitte
Comment 4 Serge Gautherie (:sgautherie) 2004-07-25 06:32:51 PDT
Created attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

Av1_Bw, updated to current trunk.
Comment 5 Serge Gautherie (:sgautherie) 2004-07-25 06:39:23 PDT
Comment on attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

Keeping
{{
(Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)	 patch 
	2003-12-06 07:58 PDT	dwitte: review+
}}

Seth:
Dan suggests giving 'rs' would be appropriate: proceed as you see fit...

Then, I'll post a full diff for checkin...
Comment 6 Serge Gautherie (:sgautherie) 2004-07-25 06:43:28 PDT
(In reply to comment #2)
> (From update of attachment 136934 [details] [diff] [review])
> >-		else {
> >+        else
> > 			m_pBuffer[offset] = *pData;
> >-		}
> >+
> 
> Also i'm no expert at coding and C(++), but this looks wrong :)

I'm only removing the brackets (and changing indentation)...
What would be wrong exactly ?
Comment 7 Serge Gautherie (:sgautherie) 2004-07-25 06:47:15 PDT
(In reply to comment #3)
> (From update of attachment 136934 [details] [diff] [review])
> nice work! is there any more of this to come for the rest of the tree?

Yes, there should be ... depending on how this (first) patch goes.
Comment 8 Serge Gautherie (:sgautherie) 2004-07-25 06:48:45 PDT
Matthias: Could you try to compile, and possibly test, this patch ? Thanks.
Comment 9 Matthias Bockelkamp 2004-10-11 13:50:37 PDT
(In reply to comment #8)
> Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

It compiles and the composer part seems to work (tested with a few signatures)
but I cannot test the import parts.
Comment 10 Serge Gautherie (:sgautherie) 2004-10-11 15:17:19 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Matthias: Could you try to compile, and possibly test, this patch ? Thanks.
> 
> It compiles and the composer part seems to work (tested with a few signatures)
> but I cannot test the import parts.

Let's be optimistic !?

Seth:
Would you have time to rs (or sr) this patch ?
Comment 11 Serge Gautherie (:sgautherie) 2005-02-01 16:09:38 PST
Comment on attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

No super-review from <sspitzer@mozilla.org> since "2004-07-25" :-(

DanM:
DanW suggests giving 'rs' would be appropriate: proceed as you see fit...
Comment 12 Dan Mosedale (:dmose) 2005-04-27 17:18:39 PDT
Comment on attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

Nice work, and sorry for the delay.  rs=dmose@mozilla.org.
Comment 13 Serge Gautherie (:sgautherie) 2005-07-31 10:02:32 PDT
Created attachment 191128 [details] [diff] [review]
(Av1b) <mailnews/*> (full diff)

Av1a-Bw, full diff for checkin,
and diffed to the current Trunk.

Keeping
{{
(Av1a-Bw) <mailnews/*> (review only)	 patch		2004-07-25 06:32 PDT   
21.24 KB	gautheri: review+
dmose: superreview+
}}

'approval1.8b4=?':
Trivial code cleanup, no risk.
Comment 14 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-08-02 05:25:43 PDT
Comment on attachment 191128 [details] [diff] [review]
(Av1b) <mailnews/*> (full diff)

I'm concerned that we're replacing tabs with newlines out of context here...
let's hold off and do this in 1.9, but I would really prefer that we use tabs
in context unless we're really going to fix up blocks of whitespace together.
Comment 15 Serge Gautherie (:sgautherie) 2012-01-07 08:43:27 PST
Created attachment 586694 [details] [diff] [review]
(Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/
[Checked in: Comment 17]

Av1b, unbitrotted.
"99%" of comment 14 suggestion(s) has been done in the meantime.

Included logic rewrites are:
*nsMsgCompose.cpp: merges 2 |*wPtr = ...|.
*nsEudoraCompose::FindNextEndLine(): moves 2 |(count < len)| to front.
*nsEudoraMailbox::ExamineAttachment(): moves 1 |(cnt < len)| to front.

(Mostly) Already r+ and rs+, but as that was 6.5 years ago...
Comment 16 Serge Gautherie (:sgautherie) 2012-01-07 08:44:50 PST
Comment on attachment 586694 [details] [diff] [review]
(Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/
[Checked in: Comment 17]

Succeeded as
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=cb3971f9652d
Comment 17 Serge Gautherie (:sgautherie) 2012-02-20 14:25:09 PST
Comment on attachment 586694 [details] [diff] [review]
(Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/
[Checked in: Comment 17]

http://hg.mozilla.org/comm-central/rev/dba9d75af64f
Comment 18 Serge Gautherie (:sgautherie) 2012-02-20 18:22:14 PST
Ftr, as these seem fine as is:

http://mxr.mozilla.org/comm-central/search?string=0x0a&find=%2Fmailnews%2F
"Found 24 matching lines in 6 files"
http://mxr.mozilla.org/comm-central/search?string=0x0d&find=%2Fmailnews%2F
"Found 25 matching lines in 9 files"

V.Fixed, per MXR.

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