I still don't see the point to be honest.

Also (stemming from this) I don't understand when I should put the header into #ifdefs and when not. Because as I understand it at the moment @cgundogan proposed that anytime a module header is included it needs to be #ifdef'd by the corresponding module define. Which isn't just in my opinion, but objectively overkill (and wouldn't cut it for with this PR).

Actually, I think the rule is quite simple: if module A depends on module B, no guards around the includes of B's header(s) are needed. In case module A used module C conditionally/optionally, the header(s) of C should be guarded.

Examples of A using B(s):

• xtimer depends on periph_timer, msg, mutex, ...
• sc_netif depends on netapi, netopt, ...

Examples of A using C conditionally:

• sc_netif using netstats and l2filter
• auto_init using almost everything worth initializing...

So to my understanding (and after looking around RIOT some more), we already do escape optional headers in parts of RIOT -> see auto_init.c, sched.c, kernel_init.c, ... just to name some.

So it seemed to me, that the case with netstats as discussed in #7066 was rather an exception and a shortcoming then the rule... So considering this, I would actually opt to guard this kind of header and make it a general rule for the coding conventions for the future.

Opinions?

I agree with you @haukepetersen. The rule of thumb should be: only include what you use.

just curious: but why don't move the #ifdef into the respective header file (i.e., at the top there)?

This would avoid clumsy #ifdef in each and every C file that uses netstats or L2filter option. I agree with @miri64 on this point: it looks ugly and bloats the code. As we already see in this PR it touches many files (i.e., 4), instead of just the 2 header files: net/netstats.h and net/l2filter.h.

I agree with you @haukepetersen. The rule of thumb should be: only include what you use.

Yes. This is also a prerequisite for out-of-tree modules. The header of a module might not be available if the module (/ package) is not built.

just curious: but why don't move the #ifdef into the respective header file (i.e., at the top there)?

This would avoid clumsy #ifdef in each and every C file that uses netstats or L2filter option. I agree with @miri64 on this point: it looks ugly and bloats the code.

IMO that's the price of conditional compilation at this level.

IMO that's the price of conditional compilation at this level.

Sure, but in case of netstats and L2filter modules it could be somewhat optimized (?) by putting the conditions directly into the header files. However, you are right: for external packages this would not work.

Sure, but in case of netstats and L2filter modules it could be somewhat optimized (?) by putting the conditions directly into the header files.

It might optimize readability slightly. It would de-optimize other things. Imagine a build system that rebuilds reliably only needed stuff (not needing make clean all the time). A simple touch l2filter.h would rebuild all netdev2 using applications, not only the ones actually using l2filter.

edit

There's a reason we don't just do #include "riot.h" and let the preprocessor sort out unneeded headers...

@kaspar030 good point! Unnecessary rebuilds are suboptimal ...

So do I read it correctly here, that we close in to a common understanding of #ifdefing out optional headers as done in this PR?

Begrudgingly 👍

I added a bullet point to the coding conventions: https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#includes -> feel free to improve!

So anyone now willing to ACK this PR?

fixed issue with gnrc_ipv6_netif...

fixed newline issue

&go.