by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCH 1/8] cfg80211: fix possible compile warning in wext-compat.h

On Wed, 2014-04-09 at 09:28 +0800, Zhao, Gang wrote:
> Function cfg80211_wext_freq() is declared in wext-compat.h, but its
> parameter struct wiphy's declaration is not included there. Actually
> the parameter is not used in this function, so change it to normal
> net_device.

I applied this, but the netdev argument is also unused so I just removed
it entirely.

johannes

2014-04-09 15:22:37

by Zhao, Gang

[permalink] [raw]

Subject: Re: [PATCH 8/8] cfg80211: remove unnecessary include clauses

On Wed, 2014-04-09 at 14:36:59 +0200, Johannes Berg wrote:
> On Wed, 2014-04-09 at 20:25 +0800, Zhao, Gang wrote:
>> On Wed, 2014-04-09 at 09:49:30 +0200, Johannes Berg wrote:
>> > I don't like those last four patches, I'd rather have more includes than
>> > rely on other headers including headers that we need - that could change
>> > after all.
>>
>> As I said in commit log, duplicate including could hide some warnings.
>
> Well, the commit logs for these patches didn't have any such
> information, but that's not really the point. Besides, what does "could
> hide some warnings" even mean? This isn't a meaningful thing, if you
> include the right headers then you should get what you need, and you
> should include the headers for what you need.

I said it in those four "fix" patches, not here. I thought it's
consensus to exclude directly included headers if it's indirectly
included in other headers, but apparently I'm wrong.

What I mean the duplicate including supressed the warnings is:

#include <a.h>
#include <b.h>
#include <c.h>

<c.h> includes <a.h>, so I thought <a.h> is "duplicate". BTW now I'm not
sure my defination of duplicate is equal to `make includecheck` command.

If <b.h> needs to includes <a.h> but forgot to do it, it will not
generate warning, since <a.h> is above, and do the work. If we remove
<a.h>, the warning will show up, and we can fix it. That's what I think
is the benifit of removing duplicate include.

>
>> In theory, the possibility of change is equal, either it's a directly
>> included header or a indirectly included header, and it's more likely to
>> change to include more, not less. So the change may not cause any
>> problem.
>
> At the current point in time. If some of the headers that you rely on
> including something no longer does in the future because it no longer
> needs that, then you just broke everything. That's the point you
> seemingly didn't consider, and I think it's much better to include what
> you need rather than rely on somebody else doing it for you. The latter
> can even be architecture-specific.

I see your point. The benifit of removing duplicate including is above,
the drawback is that we may more depend on other parts of the code than
we should be.

>
>> In other way, the local directory header files may change more
>> frequently than the header files in include/ directory. Whether it's a
>> total win to apply the last four patches may be a question, but it's
>> just amusing to see that lots of lines can be deleted. :-)
>
> # of lines isn't a relevant metric though. If you were removing includes
> that we didn't need that's certainly a useful thing, but removing
> includes because they're indirectly already included is IMHO practically
> always bad.
>
> johannes