Replace 0x0a by nsCRT::LF, 0x0d by nsCRT::CR

VERIFIED FIXED in Thunderbird 13.0

Status

MailNews Core
Backend
P4
trivial
VERIFIED FIXED
14 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Trunk
Thunderbird 13.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Updated

14 years ago
Assignee: general → gautheri
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Summary: (Code cleanup) Replace 0x0a/0x0d by nsCRT::CR/nsCRT::LF → (Code cleanup) Replace 0x0a by nsCRT::LF, and 0x0d by nsCRT::CR
(Assignee)

Comment 1

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

Updated

14 years ago
Attachment #136934 - Flags: review?(dwitte)
(Assignee)

Updated

14 years ago
Attachment #136934 - Attachment description: (Av1) Fixes most '/mailnews/' occurences (review only) → (Av1_Bw) Fixes most '/mailnews/' occurences (review only)

Comment 2

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

Updated

14 years ago
Target Milestone: --- → mozilla1.7alpha
(Assignee)

Updated

14 years ago
Summary: (Code cleanup) Replace 0x0a by nsCRT::LF, and 0x0d by nsCRT::CR → (Code cleanup) Replace 0x0a/'\n' by nsCRT::LF, and 0x0d/'\r' by nsCRT::CR
(Assignee)

Updated

14 years ago
Attachment #136934 - Attachment description: (Av1_Bw) Fixes most '/mailnews/' occurences (review only) → (Av1_Bw) Fixes most '/mailnews/' '0x0?' occurences (review only)
(Assignee)

Updated

14 years ago
Target Milestone: mozilla1.7alpha → ---

Comment 3

13 years ago
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
Attachment #136934 - Flags: review?(dwitte) → review+
(Assignee)

Comment 4

13 years ago
Created attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

Av1_Bw, updated to current trunk.
Attachment #136934 - Attachment is obsolete: true
(Assignee)

Comment 5

13 years ago
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...
Attachment #154285 - Flags: superreview?(sspitzer)
Attachment #154285 - Flags: review+
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

13 years ago
Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

Comment 9

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

Comment 10

13 years ago
(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 ?
Product: Browser → Seamonkey
(Assignee)

Comment 11

13 years ago
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...
Attachment #154285 - Flags: superreview?(sspitzer) → superreview?(dmose)
Comment on attachment 154285 [details] [diff] [review]
(Av1a-Bw) <mailnews/*> (review only)

Nice work, and sorry for the delay.  rs=dmose@mozilla.org.
Attachment #154285 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 13

12 years ago
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.
Attachment #154285 - Attachment is obsolete: true
Attachment #191128 - Flags: approval1.8b4?
(Assignee)

Updated

12 years ago
Attachment #191128 - Flags: superreview+
Attachment #191128 - Flags: review+

Comment 14

12 years ago
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.
Attachment #191128 - Flags: approval1.8b4? → approval1.8b4-

Updated

8 years ago
Component: General → Backend
Product: SeaMonkey → MailNews Core
QA Contact: general → backend
(Assignee)

Comment 15

6 years ago
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...
Attachment #191128 - Attachment is obsolete: true
Attachment #586694 - Flags: review?(mbanner)
(Assignee)

Updated

6 years ago
Attachment #586694 - Attachment description: (Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/. r=dwitte rs=dmose. → (Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/
(Assignee)

Comment 16

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

Updated

6 years ago
Priority: -- → P4
Attachment #586694 - Flags: review?(mbanner) → review+
(Assignee)

Updated

6 years ago
Attachment #154285 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #191128 - Flags: superreview+
Attachment #191128 - Flags: review+
(Assignee)

Comment 17

6 years ago
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
Attachment #586694 - Attachment description: (Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/ → (Av1c) Replace 0x0A by nsCRT::LF, 0x0D by nsCRT::CR, 9 by '\t', and 32 by ' ', in mailnews/ [Checked in: Comment 17]
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Summary: (Code cleanup) Replace 0x0a/'\n' by nsCRT::LF, and 0x0d/'\r' by nsCRT::CR → Replace 0x0a by nsCRT::LF, 0x0d by nsCRT::CR
Target Milestone: --- → Thunderbird 13.0
(Assignee)

Comment 18

6 years ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Blocks: 728991
You need to log in before you can comment on or make changes to this bug.