1403660 - Do not use 'else' after 'break' in /memory/build/rb.h

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Jan Keromnes [:janx]]

While inspecting patches, our static analyzer found a readability issue in the 'rb_remove' macro in /memory/build/rb.h:

> In /memory/build/rb.h:
> @@ -622,14 +622,13 @@
>                               rbp_r_t);                                 \
>                         }                                               \
>                         break;                                          \
>                     } else {                                            \
>                       ^
> Warning: Do not use 'else' after 'break' [clang-tidy: readability-else-after-return]

This code could indeed be simplified by removing the unnecessary 'else'.

Comment 1

[Jan Keromnes [:janx]]

Attachment: Do not use 'else' after 'break' in /memory/build/rb.h.

Please have a look.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eadf573483a

Comment 2

[Jan Keromnes [:janx]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aa5b6aab773

(I cancelled the previous one, as it seems there was a race condition that included many other changesets in my push.)

Comment 3

[Eric Rahm [:erahm]]

glandium is completely changing this code over in bug 1403444, maybe ping him there?

Comment 4

[Jan Keromnes [:janx]]

Indeed, thanks!

Glandium, feel free to take over this bug and steal my fix for the 'rb_remove' macro.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8912825 [details] [diff] [review]
Do not use 'else' after 'break' in /memory/build/rb.h.

Review of attachment 8912825 [details] [diff] [review]:
-----------------------------------------------------------------

The else is still there after bug 1403444, but please rebase on top of that bug.

Comment 6

[Jan Keromnes [:janx]]

(In reply to Mike Hommey [:glandium] from comment #5)
> The else is still there after bug 1403444, but please rebase on top of that
> bug.

I will, thanks!

Comment 7

[Jan Keromnes [:janx]]

Attachment: Do not use 'else' after 'break' in /memory/build/rb.h.

Rebased on top of bug 1403444.

Comment 8

[Jan Keromnes [:janx]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1efb1af4d04fdce530006346466c29b29d33876e

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8913230 [details] [diff] [review]
Do not use 'else' after 'break' in /memory/build/rb.h.

Review of attachment 8913230 [details] [diff] [review]:
-----------------------------------------------------------------

This is harmless, but the Mozilla Coding Style (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style) doesn't say anything about no-else-after-break, it only bans no-else-after-return. I can see that clang-tidy is lumping no-else-after-break into its "readability-else-after-return" check.

Comment 10

[Jan Keromnes [:janx]]

Thanks!

> This is harmless, but the Mozilla Coding Style (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style) doesn't say anything about no-else-after-break, it only bans no-else-after-return. I can see that clang-tidy is lumping no-else-after-break into its "readability-else-after-return" check.

Good catch. I've updated the MDN page to add "(or a `break`)" to clarify.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c93d12aca502
Do not use 'else' after 'break' in /memory/build/rb.h. r=njn

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c93d12aca502