Closed Bug 728991 Opened 8 years ago Closed 8 years ago

Replace \n by nsCRT::LF, \r by nsCRT::CR

Categories

(MailNews Core :: Backend, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: sgautherie, Unassigned)

References

Details

(Whiteboard: [good first bug][mentor=sgautherie][lang=c++] [meta])

Attachments

(2 files, 1 obsolete file)

Probably not all should be converted, especially line endings.
Yet, some would be good to do, as in bug 227633.

http://mxr.mozilla.org/comm-central/search?string=\\r&regexp=on&find=%2Fmailnews%2F
"Found 442 matching lines in 113 files"

http://mxr.mozilla.org/comm-central/search?string=\\n&regexp=1&find=%2Fmailnews%2F
"Too many hits, displaying the first 1000"
Summary: 227633 - Replace \n by nsCRT::LF, \r by nsCRT::CR → Replace \n by nsCRT::LF, \r by nsCRT::CR
One way to start this could be to look at the lonely '\r' and '\n' in
http://mxr.mozilla.org/comm-central/search?string=\\r&regexp=1&find=%2Fmailnews%2F.*\.cpp
"Found 275 matching lines in 72 files"
Whiteboard: [good first bug][mentor=sgautherie][lang=js] [meta] → [good first bug][mentor=sgautherie][lang=c++] [meta]
Is it OK to write a program to do this replacement?
I'd suggest using a script or something similar. Something like the one in attachment 653946 [details] (used for bug 579517) could be a good starting point.
What would be to win from this change? I don't see it.
(In reply to Serge Gautherie (:sgautherie) from comment #0)
> Probably not all should be converted, especially line endings.
> Yet, some would be good to do, as in bug 227633.

(In reply to Magnus Melin from comment #4)
> What would be to win from this change? I don't see it.

See bug 227633 comment 0.
That comment is only about 0x0A and 0x0D which indeed is unreadable. \n and \r on the other hand are basic knowledge and leaves no room for typos etc.
Attached file simple patch (obsolete) —
Is that what we need?
Attachment #659982 - Flags: review?(sgautherie.bz)
(In reply to WangJun from comment #7)
> Is that what we need?

here is a basic file:
'\r'
'\n'
"\r"
"\n"
"\r\n"

what will do your script:

'nsCRT::CR'
'nsCRT::LF'
"nsCRT::CR"
"nsCRT::LF"
"nsCRT::CRnsCRT::LF"

which is not what is wanted as it will keep the \" or \'
and in the example i gave i guess we cant have nsCRT::CRnsCRT::LF without any separator?
but maybe is there another substitution for "\r\n"

im working on a simple python script that can do that
as i dont like shell scripts with their \\\\\\\ for a simple quote... :)

so what is the behaviour for "\r\n"?
Attachment #659982 - Attachment is patch: false
Script done
no special behaviour on "\r\n" as it is not defuned in the enum
work with both "\r" '\r' and "\r" '\n'
try a build and if it correctly works i will push the script and the patch
Script done
no special behaviour on "\r\n" as it is not defined in the enum
work with both "\r" '\r' and "\n" '\n'
try a build and if it correctly works i will push the script and the patch
(better this way)
back from first build
it looks that trying to replace "\r" or "\n" by nsCRT* was a mistake
unable to do something like this:
printf("%s", "\n");
that would give
printf("%s", nsCRT::LF);

as nsCRT::LF is defined like that : '\r'
it looks like the best noobie thing i ever did...

i just updated my script to avoid this
trying another build to check
push everything when its done
This is the script that performs the substitution on every .cpp of a folder
build ok
Waiting for review and TryServer
Attachment #659982 - Attachment is obsolete: true
Attachment #659982 - Flags: review?(sgautherie.bz)
I forgor to mention it in this patch version but it also adds #include "nsCRT.h" if its not already in files where we replace '\r' and '\n' this way the build works
Attachment #661623 - Attachment mime type: text/x-python → text/plain
I can understand "Replace 0x0a by nsCRT::LF, 0x0d by nsCRT::CR" as in bug 227633 mentioned in comment 0  But like Magnus in comment 6, I don't understand why you consider changes like this a good thing:

-  if ( *next == nullptr || **next == '\n' || **next == '\0' ) {
+  if ( *next == nullptr || **next == nsCRT::LF || **next == '\0' ) {
Attachment #661624 - Flags: review?(sgautherie.bz)
You need to make a choice to close this bug or to apply a patch.
I dont understand why the class nsCRT and specially the nsCRT::CR and nsCRT::LF enum has been done if we do not use it.
There is also the NS_LINEBREAK define that could be used and nsCRT::VTAB
By the way there are many matches for this enum in mozilla's repos so why is there both '\r' and nsCRT::CR?
But someone as to decide what to do.
Arnaud, thanks for your work on this bug, I'm sorry I haven't been able to take a look at it earlier.

Having considered the options, I'm in agreement with Kent and Magnus, '\r' and '\n' are the common c/c++ indications and replacing them moves away from that, making it more difficult for folks unfamiliar with the code base to get used to it.

This decision also reflects recent efforts in the last couple of months, where we've be moving the whole code base to use familiar c/c++ constructs rather than special types (this work is still ongoing).

Therefore I think we should won't fix this bug.

(In reply to Arnaud Sourioux [:Six] from comment #16)
> There is also the NS_LINEBREAK define that could be used and nsCRT::VTAB

Just to note that NS_LINEBREAK varies according to the platform you're on, so it isn't necessarily useful, nsCRT:VTAB isn't actually used anywhere.

> By the way there are many matches for this enum in mozilla's repos so why is
> there both '\r' and nsCRT::CR?

I only found about 60 instances of nsCRT::LF and about 50 instances of nsCRT::CR, which isn't actually that many, I think most were added by bug 227633 and in retrospect, I think we should have gone for changing to '\r' and '\n' in that bug.

Thanks again for your effort on the bug, there's many other bugs you can help with, if you have further questions or need help, feel free to ask one of us direct.

> But someone as to decide what to do.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Comment on attachment 661624 [details] [diff] [review]
Patch generated by the script to substitute '\r' and '\n' with nsCRT::CR and nsCRT::LF

Clearing obsolete review request
Attachment #661624 - Flags: review?(sgautherie.bz)
You need to log in before you can comment on or make changes to this bug.