+++ This bug was initially created as a clone of Bug #1356843 +++ nsParseMailbox.cpp:1383:21: error: possible misuse of comma operator here [-Werror,-Wcomma]

Never seen this before? Me either ;-( https://en.wikipedia.org/wiki/Comma_operator#Avoid_a_block

https://hg.mozilla.org/comm-central/rev/9978596cc5d566044597c5a13e0c7801523296ac Third bustage today.

Sadly another case seen on try.

Damn, there are many more on try: nsImapMailFolder.cpp:4275 nsImapProtocol.cpp:7908 nsBayesianFilter.cpp:457 mimeenc.cpp:193 mimeenc.cpp:362 mimeenc.cpp:594

See how we go with this: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aa9bafa6fb5f1aa7f351057c24d9ed7617cfd7b8

More. This time some real beauty in MIME.

While I'm here, I'll fix some bugs: "BLINK>" is six characters long not five, and FLUSHBOTH translates to <P ALIGH=JUSTIFY>.

More hard to read MIME code, grrr.

There were problems in Mork. Check this: void AddFirst(morkLink* in) /*i*/ { - ( (mDeque_Head.mLink_Next->mLink_Prev = - (in))->mLink_Next = mDeque_Head.mLink_Next, - ((in)->mLink_Prev = &mDeque_Head)->mLink_Next = (in) ); + (mDeque_Head.mLink_Next->mLink_Prev = in)->mLink_Next = + mDeque_Head.mLink_Next; + (in->mLink_Prev = &mDeque_Head)->mLink_Next = in; } https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=872f2e71d64a145fa23a38746a33bb4bf9640b3b

Oops, I wanted to include the patch from bug 853881 as well: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=045818b1b2299fe0c34c5bdb503b42b4423612e1

Mac built OK, but tests don't start. So another Linux try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=690d0023c7d07b0f2103ec34838bfc86787eb25f

Comment on attachment 8860447 [details] [diff] [review] 1358444-comma2.patch (v6). Review of attachment 8860447 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good to me, I don't like the comma either. Let's wait for the try run. ::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp @@ +453,5 @@ > nsCString headerValue; > nsAutoCString headerName; // we'll be normalizing all header names to lower case > bool hasMore; > > + while (NS_SUCCEEDED(aHeaderNames->HasMore(&hasMore)) && hasMore) Nice, thanks. ::: mailnews/mime/emitters/nsMimeHtmlEmitter.cpp @@ +195,5 @@ > // optimization: if we aren't in view all header view mode, we only show a small set of the total # of headers. > // don't waste time sending those out to the UI since the UI is going to ignore them anyway. > if (aHeaderMode != VIEW_ALL_HEADERS && (mFormat != nsMimeOutput::nsMimeMessageFilterSniffer)) > { > nsDependentCString headerStr(headerInfo->name); Is this used elsewhere than the else {} block? Can you push it inside? @@ +198,5 @@ > { > nsDependentCString headerStr(headerInfo->name); > + bool skip = true; > + // Accept the following: > + if (PL_strcasecmp("to", headerInfo->name) == 0) skip = false; In C++ !PL_strcasecmp() would be fine too. @@ +224,5 @@ > + else { > + // Accept if it's an "extra" header. > + // Make headerStr lower case because IndexOf is case-sensitive. > + ToLowerCase(headerStr); > + if (extraExpandedHeadersArray.Length() > 0 && Doesn't this slightly change the logic? It runs ToLowerCase(headerStr) regardless of extraExpandedHeadersArray.Length(), which wasn't the case before. It shouldn't hurt, but may be slower. It could be split into 2 ifs(). ::: mailnews/mime/src/mimetric.cpp @@ +180,5 @@ > + { tag_open = "<FONT SIZE=\"+1\">"; tag_close = "</FONT>"; } > + else if (!PL_strncasecmp ("BLINK>", old, 6)) > + // Of course, both text/richtext and text/enriched must be > + // enhanced *somehow*... Or else what would people think. > + { tag_open = "<BLINK>"; tag_close = "</BLINK>"; } Can you put the comment inside the {} ?

(In reply to Jorg K (GMT+2) from comment #8) > While I'm here, I'll fix some bugs: > "BLINK>" is six characters long not five, and FLUSHBOTH translates to <P > ALIGH=JUSTIFY>. Good catch.

Hey, thanks for the quick review, much appreciated. (In reply to :aceman from comment #14) > > nsDependentCString headerStr(headerInfo->name); > Is this used elsewhere than the else {} block? Can you push it inside? Will do. > In C++ !PL_strcasecmp() would be fine too. Not for an old man like me although I know it's used a lot. The compare function returns zero if the strings are equal, that's what I want to test. I don't want to test !(less || greater). > Doesn't this slightly change the logic? It runs ToLowerCase(headerStr) > regardless of extraExpandedHeadersArray.Length(), which wasn't the case > before. It shouldn't hurt, but may be slower. It could be split into 2 ifs(). Well, I did the whole restructuring (call it code churn) to evaluate as late as possible. I could have moved the ToLowerCase(headerStr) before the 'if'. Now you're telling me I should do even better? OK. Just for you ;-) > > + // Of course, both text/richtext and text/enriched must be > > + // enhanced *somehow*... Or else what would people think. > > + { tag_open = "<BLINK>"; tag_close = "</BLINK>"; } > Can you put the comment inside the {} ? No, because a) the comment makes no sense, and b) it will destroy the formatting.

Comments addressed.

Fiddled with that big 'if' statement a little more.

https://hg.mozilla.org/comm-central/rev/9978596cc5d566044597c5a13e0c7801523296ac (comment #3) https://hg.mozilla.org/comm-central/rev/2bf4d1d2bf58662aeb5dd8f260f83842eeb63f61