Found by http://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html

Comment on attachment 8855207 [details] [diff] [review] a.diff Review of attachment 8855207 [details] [diff] [review]: ----------------------------------------------------------------- A few comments: - You've sent a large patch here, so this will take some time to inspect thoroughly since I assume that is was done by hand. - I can see that you invested time into this, so I don't want to disappoint you, but is this really necessary? We're refactoring working code just to change the layout and appease some automated code checkers? That way, we're burning developers resources desperately needed elsewhere. We have other bugs in the pipeline changing those files and the patches prepared there will no longer apply, for example bug 1242030, 1242030-new-part-2-imap.patch. We're also making our lives real hard when it comes to uplifts. Patches on top of any refactoring or white-space changes will not apply to earlier branches. And since I'm doing those uplifts, I've just created myself more work, and worse, more risk since I'll have to manually resolve merge conflicts with the risk of getting it wrong. So I'm generally against those changes, which doesn't mean that we can accept the patch ;-) We also have something as personal coding style. To me if (condition) return 1; else return 0; may be quite acceptable in some contexts. So can you please state the motivation here. And also: How much more refactoring do you intend to do. Below a few comments by going over it for a while. ::: mailnews/imap/src/nsAutoSyncManager.cpp @@ +499,5 @@ > nsresult rv = autoSyncState->GetState(&state); > if (NS_SUCCEEDED(rv) && aState == state) > break; > + > + offset++; Something like this is OK to change. ::: mailnews/imap/src/nsIMAPGenericParser.cpp @@ +213,2 @@ > return CreateQuoted(); // quoted > + return CreateAtom(true); // atom Original was more legible. ::: mailnews/imap/src/nsIMAPNamespace.cpp @@ +349,5 @@ > + char *ourstr = PL_strdup(str); > + char *origOurStr = ourstr; > + if (ourstr) > + { > + char *token = NS_strtok(SERIALIZER_SEPARATORS, &ourstr ); Why increase the indent here? Shouldn't it be two spaces? @@ +360,2 @@ > if (where[PL_strlen(where)-1] == '"') > + where[PL_strlen(where)-1] = 0; And here. ::: mailnews/imap/src/nsImapMailFolder.cpp @@ +862,5 @@ > { > ThrowAlertMsg("folderExists", msgWindow); > return NS_MSG_FOLDER_EXISTS; > } > + if (mIsServer && folderName.LowerCaseEqualsLiteral("inbox")) // Inbox, a special folder I'd say you're reducing legibility here. @@ +2152,5 @@ > { > curSequenceEnd = nextKey; > continue; > } > + if (curSequenceEnd > startSequence) And here. ::: mailnews/imap/src/nsImapServerResponseParser.cpp @@ +2732,5 @@ > } > > if (fNextToken[1] == '(') > return bodystructure_multipart(partNum, parentPart); > + return bodystructure_leaf(partNum, parentPart); I think the original reads better then this.

Aceman, do you have an opinion here?

> - You've sent a large patch here, so this will take some time to inspect > thoroughly since > I assume that is was done by hand. Nope, automatically, indentation was done by hand (as clang format would break everything) > - I can see that you invested time into this, so I don't want to disappoint > you, but > is this really necessary? Firefox devs considered that it was worth it: https://bugzilla.mozilla.org/show_bug.cgi?id=1338086 It is decreasing the complexity of the code. If you are not happy about the changes, just mark it as wontfix. No worries

Let me look at them on a rainy Sunday afternoon. Too many other fires burning right now.

It could be useful but I wouldn't reindent some of the larger blocks and also would add an empty space before the pattern: - else if () { + if () { Of course, this should be done after some of the feature patches are landed.

Can we land this change or should I just wontfix it?

It's still on the radar. Is there a rush to do it? I'd be taking a good part of the patch (which most likely has bit-rot by now). Can you leave it with me?

If refreshed the patch, 99% still applied. I'll go thought within one week and tweak it to my liking, OK?

Well, I am still interested to continue the work I started. Could you define your "liking" ? There is no rush, just reported 4months ago and this is a trivial change.

(In reply to Sylvestre Ledru [:sylvestre] from comment #10) > Well, I am still interested to continue the work I started. Could you define > your "liking" ? Yes. I like to avoid unrelated white space changes, and I added some blank lines. That's all. I'll attach a slightly modified patch in a minute.

That's all the hunks apart from nsIMAPBodyShell.cpp which I haven't had time to check yet.

(In reply to Jorg K (GMT+2) from comment #11) > I like to avoid unrelated white space changes, ... So I removed hunks that removed trailing white-space. I did however change one whole block to indent with two instead of four spaces. All a matter of taste ;-) I'll look that the missing nsIMAPBodyShell.cpp as promised, but that has 500+ lines of changes.

Comment on attachment 8889517 [details] [diff] [review] 1354011-else-case.patch - part 1 I think this is OK now ;-) I'll do the second part soon.

https://hg.mozilla.org/comm-central/rev/84b3fef242e44421c7327cf23c3f5eeeb13e2d03 Bug 1354011 - Remove useless else blocks in order to reduce complexity in mailnews/imap/ r=jorgk

Here's the second part. I've made some white-space adjustments and I'll take a good look at it in the next few days.

White-space changes.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/fb6824894528 Remove useless else blocks in order to reduce complexity in mailnews/imap/ (part 2). r=jorgk

Comment on attachment 8889615 [details] [diff] [review] 1354011-else-case-part2.patch I went ahead and pushed this.