81827 - Fix style inconsistencies in nsMsgSend.cpp

Status: RESOLVED WONTFIX

(MailNews Core :: Composition, defect)

Description

[Blake Ross]

Areas that need fixing include:

* Tab removal
* Proper and consistent indentation in accordance with the modeline
* Consistent wrapping of |if| (and other) conditions in spaces

Specifically, the minor problems introduced in the patch for 81720.

Comment 1

[Jean-Francois Ducarroz]

accepting

Comment 2

[Blake Ross]

Oops, I guess my caring about this 'trivial' sort of thing makes me an idiot.  
I guess the mail team is content with the low number of outside contributions 
they get.  So wontfix it is.

Comment 3

[Jean-Francois Ducarroz]

What the f... are you doing, can stop acting like a child! Please stop changing bug resolution without a good reason, 
we are working here, not playing!!!

I have accepted the bug, I'll convert those old remainings tabs next time I have to change something in this file. 

Comment 4

[lchiang]

Blake - you've filed a bug for correcting the inconsistencies.  Let the mail
team schedule their work on this along with all the other priorities they have
in their work.  JF has already "accepted" the bug and didn't close it as won't fix. 

This has nothing to do with contributions to the mail code.  

Comment 5

[Andreas Kunz]

Attachment: nsMsgSend.cpp cleanup patch

Ducarroz, since you explicitly did un-WONTFIX this bug, I put some work into it
to let you make more creative work than this quite mechanical fix...
This patch makes nsMsgSend.cpp much cleaner...:

First I used the C++ beautifier 'bcpp-20021124' to remove the tabs and do all
the indentation and the correct placement of curly brackets.
Of course I used the dominating 2-spaces-indentation and putting the curly
bracket at the beginning of a function body or an "if"-body in its own line. 
This solves problems 1 and 2 completely (the modeline says blocks should be
indented by 2 spaces and no tabs should be used for indentation; *if* a tab is
used somewhere it should be four spaces wide).

Afterwards I compared the whole file with the original (by toggling the
document displayed in my text editor so no change could go unseen) to look at
each change made, correct the ones that were suboptimal (multi-lines-function
calls, some comments) and make sure nothing got worse.

Finally I manually adjusted the 17 
"if ( blabla..." 
occurances to make them look like the 469 
"if (blabla..." 
ones and checked that "while ( blabla" does occur nowhere. This pretty much
solves problem 3. (For a special case please see my next comment.)

While doing that manual part, I found two typos I just could not refuse to
correct:
line 708 (original file): "attachement" -> "attachment"
line 1107: "heirarchy" -> "hierarchy"
And I adjusted a few comments to line up properly (only ones that are typically
seen simultaneously). Generally I did touch comments only where really
necessary.

To make it clear again: this patch are only whitespace changes (apart from the
two typos).

This patch does not make the file perfectly clean, but certainly much cleaner
than most other files - and I think it is sufficient to fix this bug.

Comment 6

[Andreas Kunz]

Comment on attachment 116895 [details] [diff] [review]
nsMsgSend.cpp cleanup patch

J. F. Ducarroz, can you review this? Might save you some work.

(Almost) only whitespace changes, but LOTS of it...
That beautifier also removed trailing spaces, so if you see a line in the patch
replaced by a seemingly identical line, the original had an unneeded space at
its end. Makes the diff much larger, but does not hurt (or does it?).

One thing I'd like you to look at more closely is line 2986 (and line 3211) in
the original file: 
if ( m_deliver_mode != nsMsgSaveAsDraft && m_deliver_mode !=
nsMsgSaveAsTemplate )
is not easily comprehensible if you are not sure about the operator precedence
rules... so I better left those two lines alone to make them not look worse.
But of course you can tell me if I should change them (and how).

Please also tell me if this needs sr= (I guess yes: although it's whitespace
only, it's quite large...) and if you can checkin yourself (I have no
privileges).
Thanks.

Comment 7

[Jean-Francois Ducarroz]

Comment on attachment 116895 [details] [diff] [review]
nsMsgSend.cpp cleanup patch

I'll let ssu do the review...

Comment 8

[Andreas Kunz]

Oooops, forgot to cc myself...
Ok. If you want me to remove the changes that only remove trailing spaces, I
could do that. They don't hurt, but make the review 'a bit' lengthy...
And if you want other things to be changed... well, I'm not a perfect C++
beautifier...

Comment 9

[David :Bienvenu]

Comment on attachment 116895 [details] [diff] [review]
nsMsgSend.cpp cleanup patch

thx for fixing the braces!

Comment 10

[Andreas Kunz]

I don't think the 42 changes that went into nsMsgSend.cpp since I wrote this
huge patch do help much with respect to a problem-free checkin.

Whoever likes dealing with whatever happens when one tries to check this in, may
do so, since I do not have cvs access.

Comment 11

[Wayne Mery (:wsmwk)]

assuming this never got checked in.
over to Gary

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #11)
> assuming this never got checked in.

Maybe Serge knows.

That said, this patch has rotted, assuming it never got checked in:

$ patch -p0 --dry-run < ~/Desktop/81827v1.diff 
patching file nsMsgSend.cpp
Hunk #1 FAILED at 139.
Hunk #2 succeeded at 156 (offset -29 lines).
Hunk #3 FAILED at 227.
Hunk #4 FAILED at 279.
Hunk #5 FAILED at 296.
Hunk #6 FAILED at 305.
Hunk #7 FAILED at 322.
Hunk #8 FAILED at 358.
Hunk #9 FAILED at 379.
Hunk #10 FAILED at 416.
Hunk #11 FAILED at 429.
Hunk #12 FAILED at 451.
Hunk #13 FAILED at 465.
Hunk #14 succeeded at 428 (offset -87 lines).
Hunk #15 FAILED at 437.
Hunk #16 FAILED at 446.
Hunk #17 FAILED at 461.
Hunk #18 FAILED at 470.
Hunk #19 FAILED at 490.
Hunk #20 FAILED at 502.
Hunk #21 FAILED at 524.
Hunk #22 FAILED at 541.
Hunk #23 FAILED at 555.
Hunk #24 FAILED at 582.
Hunk #25 succeeded at 596 (offset -106 lines).
Hunk #26 FAILED at 627.
Hunk #27 FAILED at 645.
Hunk #28 FAILED at 674.
Hunk #29 FAILED at 690.
Hunk #30 FAILED at 710.
Hunk #31 FAILED at 723.
Hunk #32 FAILED at 777.
Hunk #33 FAILED at 820.
Hunk #34 FAILED at 857.
Hunk #35 FAILED at 878.
Hunk #36 FAILED at 906.
Hunk #37 succeeded at 966 (offset -93 lines).
Hunk #38 succeeded at 990 (offset -93 lines).
Hunk #39 succeeded at 1012 (offset -93 lines).
Hunk #40 FAILED at 1033.
Hunk #41 FAILED at 1048.
Hunk #42 FAILED at 1068.
Hunk #43 FAILED at 1089.
Hunk #44 FAILED at 1111.
Hunk #45 FAILED at 1130.
Hunk #46 FAILED at 1138.
Hunk #47 FAILED at 1157.
Hunk #48 FAILED at 1165.
Hunk #49 FAILED at 1186.
Hunk #50 FAILED at 1206.
Hunk #51 FAILED at 1222.
Hunk #52 succeeded at 1241 (offset -86 lines).
Hunk #53 FAILED at 1251.
Hunk #54 FAILED at 1267.
Hunk #55 FAILED at 1286.
Hunk #56 FAILED at 1335.
Hunk #57 FAILED at 1354.
Hunk #58 FAILED at 1379.
Hunk #59 FAILED at 1395.
Hunk #60 FAILED at 1406.
Hunk #61 FAILED at 1424.
Hunk #62 FAILED at 1450.
Hunk #63 FAILED at 1473.
Hunk #64 FAILED at 1488.
Hunk #65 FAILED at 1501.
Hunk #66 FAILED at 1526.
Hunk #67 FAILED at 1570.
Hunk #68 FAILED at 1605.
Hunk #69 FAILED at 1615.
Hunk #70 FAILED at 1632.
Hunk #71 FAILED at 1650.
Hunk #72 FAILED at 1662.
Hunk #73 FAILED at 1672.
Hunk #74 FAILED at 1712.
Hunk #75 FAILED at 1726.
Hunk #76 FAILED at 1740.
Hunk #77 FAILED at 1762.
Hunk #78 FAILED at 1774.
Hunk #79 FAILED at 1814.
Hunk #80 FAILED at 1859.
Hunk #81 FAILED at 1888.
Hunk #82 FAILED at 1934.
Hunk #83 FAILED at 1949.
Hunk #84 FAILED at 1977.
Hunk #85 FAILED at 1992.
Hunk #86 FAILED at 2037.
Hunk #87 FAILED at 2082.
Hunk #88 FAILED at 2119.
Hunk #89 FAILED at 2130.
Hunk #90 FAILED at 2174.
Hunk #91 FAILED at 2186.
Hunk #92 FAILED at 2333.
Hunk #93 succeeded at 2412 (offset -15 lines).
Hunk #94 FAILED at 2439.
Hunk #95 FAILED at 2461.
Hunk #96 FAILED at 2479.
Hunk #97 FAILED at 2494.
Hunk #98 FAILED at 2511.
Hunk #99 FAILED at 2520.
Hunk #100 FAILED at 2552.
Hunk #101 succeeded at 2599 (offset -16 lines).
Hunk #102 FAILED at 2622.
Hunk #103 FAILED at 2630.
Hunk #104 FAILED at 2668.
Hunk #105 FAILED at 2693.
Hunk #106 FAILED at 2721.
Hunk #107 FAILED at 2737.
Hunk #108 FAILED at 2775.
Hunk #109 FAILED at 2789.
Hunk #110 FAILED at 2836.
Hunk #111 FAILED at 2877.
Hunk #112 succeeded at 2968 (offset 49 lines).
Hunk #113 FAILED at 3027.
Hunk #114 FAILED at 3044.
Hunk #115 FAILED at 3054.
Hunk #116 FAILED at 3079.
Hunk #117 FAILED at 3109.
Hunk #118 FAILED at 3128.
Hunk #119 FAILED at 3154.
Hunk #120 FAILED at 3173.
Hunk #121 FAILED at 3192.
Hunk #122 FAILED at 3236.
Hunk #123 FAILED at 3263.
Hunk #124 FAILED at 3295.
Hunk #125 FAILED at 3323.
Hunk #126 FAILED at 3375.
Hunk #127 FAILED at 3401.
Hunk #128 FAILED at 3418.
Hunk #129 FAILED at 3438.
Hunk #130 FAILED at 3452.
Hunk #131 FAILED at 3481.
Hunk #132 FAILED at 3493.
Hunk #133 FAILED at 3504.
Hunk #134 FAILED at 3585.
Hunk #135 FAILED at 3623.
Hunk #136 FAILED at 3649.
Hunk #137 FAILED at 3667.
Hunk #138 FAILED at 3681.
Hunk #139 FAILED at 3735.
Hunk #140 FAILED at 3747.
Hunk #141 FAILED at 3766.
Hunk #142 FAILED at 3808.
Hunk #143 FAILED at 3826.
Hunk #144 succeeded at 4103 (offset 318 lines).
Hunk #145 FAILED at 4114.
Hunk #146 succeeded at 4178 (offset 322 lines).
Hunk #147 FAILED at 4191.
Hunk #148 FAILED at 4205.
Hunk #149 FAILED at 4255.
Hunk #150 FAILED at 4274.
Hunk #151 FAILED at 4297.
Hunk #152 FAILED at 4368.
Hunk #153 FAILED at 4390.
Hunk #154 FAILED at 4411.
Hunk #155 FAILED at 4424.
Hunk #156 FAILED at 4452.
Hunk #157 FAILED at 4490.
Hunk #158 FAILED at 4502.
Hunk #159 FAILED at 4519.
Hunk #160 FAILED at 4534.
Hunk #161 FAILED at 4545.
Hunk #162 FAILED at 4558.
Hunk #163 FAILED at 4608.
Hunk #164 FAILED at 4620.
Hunk #165 FAILED at 4639.
Hunk #166 FAILED at 4668.
Hunk #167 FAILED at 4691.
Hunk #168 FAILED at 4766.
Hunk #169 succeeded at 4829 with fuzz 2 (offset 297 lines).
Hunk #170 FAILED at 4874.
Hunk #171 FAILED at 4882.
Hunk #172 FAILED at 4897.
Hunk #173 FAILED at 4907.
Hunk #174 FAILED at 4928.
Hunk #175 FAILED at 4954.
Hunk #176 FAILED at 4991.
Hunk #177 FAILED at 5016.
Hunk #178 succeeded at 5043 (offset 292 lines).
Hunk #179 succeeded at 5071 (offset 291 lines).
Hunk #180 succeeded at 5078 with fuzz 1 (offset 290 lines).
Hunk #181 succeeded at 5110 (offset 289 lines).
164 out of 181 hunks FAILED -- saving rejects to file nsMsgSend.cpp.rej

Comment 13

[:aceman]

Can we make something like this a [good first bug] ?

Comment 14

[Magnus Melin [:mkmelin]]

Probably not worth it. I think jcranmer said he'll rewrite the whole file.

Comment 15

[Jorg K (CEST = GMT+2)]

Trailing spaces are gone now anyway and the whole file is different now, like |#if defined(DEBUG_ducarroz)| is gone, etc.