I don't like macros... But we can discuss this again.

If we go for this, I'd opt for "ARRAY_SIZE()". It's what Linux is using.

@benpicco Did you know that Coccinelle (Semantic Patch) has an example precisely for this?

Have a look at: http://coccinelle.lip6.fr/rules/

In fact, this is my first experience with spatch and I must say that I'm impressed.

You may have missed a few more chances for ARRAY_SIZE.

	modified:   boards/common/saml1x/include/periph_conf.h
	modified:   boards/same54-xpro/include/periph_conf.h
	modified:   boards/saml21-xpro/include/periph_conf.h
	modified:   boards/samr30-xpro/include/periph_conf.h
	modified:   cpu/mips_pic32_common/periph/gpio.c
	modified:   tests/libfixmath/main.c
	modified:   tests/lua_loader/main.c
	modified:   tests/periph_uart/main.c
	modified:   tests/unittests/tests-div/tests-div.c

The .cocci file I used is:

// Use the macro ARRAY_SIZE when possible
//
// Confidence: High
// Copyright: (C) Gilles Muller, Julia Lawall, EMN, INRIA, DIKU.  GPLv2.
// URL: http://coccinelle.lip6.fr/rules/array.html
// Options: -I ... -all_includes can give more complete results

@i@
@@

#include "kernel_defines.h"

@depends on i@
type T;
T[] E;
@@

- (sizeof(E)/sizeof(*E))
+ ARRAY_SIZE(E)


@depends on i@
type T;
T[] E;
@@

- (sizeof(E)/sizeof(E[...]))
+ ARRAY_SIZE(E)

spatch can even do better by adding this

@depends on i@
type T;
T[] E;
@@

- (sizeof(E)/sizeof(T))
+ ARRAY_SIZE(E)

It then finds possible changes like this:

diff --git a/examples/dtls-echo/dtls-server.c b/examples/dtls-echo/dtls-server.c
index aab5290ae..14a69dcff 100644
--- a/examples/dtls-echo/dtls-server.c
+++ b/examples/dtls-echo/dtls-server.c
@@ -210,7 +210,7 @@ static int _peer_get_psk_info_handler(struct dtls_context_t *ctx, const session_
 
     if (id) {
         uint8_t i;
-        for (i = 0; i < sizeof(psk) / sizeof(struct keymap_t); i++) {
+        for (i = 0; i < ARRAY_SIZE(psk); i++) {
             if (id_len == psk[i].id_length && memcmp(id, psk[i].id, id_len) == 0) {
                 if (result_length < psk[i].key_length) {
                     dtls_warn("buffer too small for PSK");

Notice that your sed command can be improved a bit.

• the spaces around the divide should be optional
• the space does not need an escape
• it's safer to match for non-) and have at least one character in the first sizeof, hence the [^)]+

Try this and you'll have more hits, for example in drivers/ltc4150/ltc4150_last_minute.c and sys/auto_init/netif/auto_init_cc110x.c. But there are more.

sed -ri 's/sizeof\(([^)]+)\) ?\/ ?sizeof\(\1\[0\]\)/ARRAYSIZE\(\1\)/g'

@benpicco Have a look at my add-arraysize-macro branch. It's not a nice PR. Consider it just a hint for more changes that could be done.

https://github.com/keestux/RIOT/tree/add-arraysize-macro

spatch could not help anymore due to various limitations (type qualifiers and such). I've used regex greps to find potential places and then looked at them individually.

There's one more place to look at: sys/riotboot/slot.c. I wasn't sure if that code includes the definition of ARRAY_SIZE somewhere.

sys/riotboot/slot.c is still RIOT, so I assume kernel_defines.h gets included after all.

tests/riotboot still compiles for samr21-xpro. I hope any possible breakage will be found by CI.

I added all your findings, except for tests/evtimer_msg/main.c which causes a conflict - here the macro does not apply anymore in master.

So squash?

@benpicco can you squash the commits with "fixup!"? Maybe you want to do more squashing? It's up to you.

@benpicco I've added the label for murdock. As you can see murdock failed because of a merge conflict. Can you have a look?

I would say "rebase onto master", which is probably the best strategy. No?

@benpicco I've added the label for murdock. As you can see murdock failed because of a merge conflict. Can you have a look?

I would say "rebase onto master", which is probably the best strategy. No?

Yes, but you don't need Murdock to see that. The conflict is also notified by GitHub. ;-)

I don't like macros... But we can discuss this again.

If we go for this, I'd opt for "ARRAY_SIZE()". It's what Linux is using.

I think there are macros and macros. They can be annoying and actually hinder debugging when they hide something like 10 lines of code and changes of variables. But this kind of macros with just one input and a clear output are fine (and actually helpful) IMHO.

@benpicco why is the commit 00029ec (sys/shell: remove superfluous explicit cast) necessary? I prefer to not do unrelated changes in this PR.

And why was it necessary to undo the change in sys/include/embUnit/HelperMacro.h ? It looked OK to me.

Touching all those files seems to have triggered some new Travis checks. I'll make a separate pull request for those then.

Since embUnit appears to be external code, I didn't want to touch if it's not necessary, so it's easier to import new versions in the future.

Touching all those files seems to have triggered some new Travis checks. I'll make a separate pull request for those then.

It is allowed to piggy-back style and CI fixes to a PR. No need to open a separate one.

(just keep 'em in separate commits, so 👍)

Since embUnit appears to be external code, I didn't want to touch if it's not necessary, so it's easier to import new versions in the future.

Good thinking, however it was meddled with before and I doubt there are any changes happening upstream (the last update is from 2004).

With all the required includes of kernel_defines.h one starts to wonder if that is the right place for the define of ARRAY_SIZE.

Yea that's what stopped me from fixing the remaining build errors.
Just sprinkling #include "kernel_defines.h all over the code feels kind of ugly.
I thought if I could just pluck it into some high level include all would be fine (and fragile …), but there is no such universal header file.

I think I world have less hesitation if it were just called macros.h or so.

sys/arduino/base.cpp:90:style (knownConditionTrueFalse): Condition '!(adc_line_state&(1<<arduino_pin))' is always true

Now this one I don't get.
The condition should only be always true for the first invocation, as intended.

Or is cppcheck taking that assert() into account? Here ANALOG_PIN_NUMOF could be 0 for some boards.

Rebased again & made cppcheck happy.

Great

Or is cppcheck taking that assert() into account? Here ANALOG_PIN_NUMOF could be 0 for some boards.

Yes, it takes every path in account (so the assert is ignored for the NDEBUG=1 case ;-)). You could have just add a supression comment, but your solution also looks sensible (even though non-obvious ^^")

Hmm, I should have hit the Merge button yesterday :-( But, it was late and I wanted to check one more thing.

@benpicco Can you have a look?

Let's try it again!