[U-Boot] [PATCH] flash.h: pull in common.h for types

[J. William Campbell <jwilliamcampbell@comcast.net>]

Mike Frysinger wrote:
> On Tuesday 17 November 2009 16:56:58 Wolfgang Denk wrote:
>   
>> Scott Wood wrote:
>>     
>>>> My question: is there a definitive position  somewhere  (for  example
>>>> for  the  Linux kernel; I'm sure we don't have one for U-Boot [yet]),
>>>> whether system headers should be self-sufficient?
>>>>         
>>> I'd say they should be self-sufficient, in that the inclusion of the
>>> header itself should not fail if I haven't included some arbitrary other
>>> header.  I don't see what the argument would be for not doing this.
>>>       
>> Well, Theo de Raadt says for example "... people would be able to
>> include less files; indeed, almost be careless about what they
>> include. But this would not increase portability in any way. And
>> 'make build' would probably, if it was taken the nth degree, take
>> twice as long. Therefore there is no benefit for the crazy rule you
>> suggest..." - see
>> http://www.mail-archive.com/tech at openbsd.org/msg00425.html
>>     
>
> i disagree with this using, ironically, the same base logic, but a different 
> conclusion:
> http://sourceware.org/ml/libc-alpha/2006-08/msg00064.html
>
> also, i think a self contained system like u-boot which has full control at 
> the api level can be better at this than a user interface which really sits on 
> top of an abi and has to deal with a lot of crap from user code.
>
> while i'm not asking for you or anyone else to audit header paths here as i 
> think that level of enforcement will bog things down, small patches from 
> people who choose to fix things should be merged.
>
>   
FWIW, I think one needs to be very careful with this reasoning. It is 
clear that experienced and capable programmers disagree on the correct 
approach to this problem. It is also true that the logical structure of 
the include chain is important. A "crap" interface is going to be hard 
to maintain no matter how you do it. The problem is, "crap" has a way of 
sneaking into well-designed interfaces in the form of small patches to 
"fix" things. (I am not saying this is one of them). I have observed 
that in any large program, as time goes on, more and more things get 
included in more and more places. Hiding this fact by doing the 
inclusion in other header files often obscures the drift towards a very 
polluted name space, where editing just about any include file requires 
the entire system to be rebuilt. Some would say, so what, it is easy to 
do. However, these situations often result in hard to find bugs. The 
question of "which modules can access this variable" becomes "all of 
them" because of (badly designed?)  interfaces being included in other 
interfaces without the user being explicitly aware of this. Often I see 
in patch critiques the statement "This belongs in a header file", which 
may be true, but can lead to a bunch of un-related things being stuck 
together in a header file just to meet the requirement. Those bad 
choices then get included in other header files and we are off to the 
races. Since the users are not explicitly aware they are pulling in a 
bunch of stuff that has nothing to do with the main purpose of the 
module being written, there is often no incentive to clean up bad header 
designs until things get so bad it is almost impossible to untangle 
things. So when adding an include to an include file, think hard whether 
you are bringing along a lot of things that are "not required" and that 
one would not expect to be exposed in all files using the modified 
include file. If you are bringing along a lot of "baggage", perhaps the 
interface you are including could use some re-work.

Best Regards,
Bill Campbell
>>> I don't know whether Linux has a specific policy on this, but I haven't
>>> noticed many problems in this regard, and when I did find one in the
>>> kernel a few years back I didn't get any argument when I submitted a
>>> patch to fix it.
>>>       
>
> ive semi-frequently post fixes to linux headers so that you can include just 
> that header and have it work.  i have yet to hear anyone complain; rather 
> every one has been merged (ignoring issues unrelated to the original purpose).
>
>   
>>> Which man pages are you looking at?
>>>       
>> Well, for example:
>>
>> open(2):
>> 	SYNOPSIS
>> 	       #include <sys/types.h>
>> 	       #include <sys/stat.h>
>> 	       #include <fcntl.h>
>>
>> mknod(2):
>> 	SYNOPSIS
>> 	       #include <sys/types.h>
>> 	       #include <sys/stat.h>
>> 	       #include <fcntl.h>
>> 	       #include <unistd.h>
>>
>> stat(2):
>> 	SYNOPSIS
>> 	       #include <sys/types.h>
>> 	       #include <sys/stat.h>
>> 	       #include <unistd.h>
>>
>> Why do we need these lists of #includes? WHy doe - for example -
>> <sys/stat.h> not auto-include anything it might need?
>>
>> To me this seems to be an indication that there is no intention to
>> make headers self-sufficent, but I am absolutely not sure.
>>     
>
> i'm pretty sure your man page example is an unrelated issue.  the include list 
> does not imply that any one of those headers cannot be included by itself 
> first.  if you read the full text of the man page, there are many 
> defines/options which may be utilized.  if you want to use different pieces, 
> you might have to include the related header.  so the full header list is 
> given to cover any one piece of code in the man page by itself.
>
> going by my point, none of these will result in a build failure:
> for h in sys/types.h sys/stat.h unistd.h fcntl.h ; do
> 	gcc -include $h -x c -c - -o /dev/null < /dev/null
> done
>
> going by the man page point of open(2), this code will compile:
> #include <fcntl.h>
> main() { open("foo", O_RDONLY); }
> but if you want to create a file with symbolic perms, you need sys/stat.h:
> #include <fcntl.h>
> #include <sys/stat.h>
> main() { open("foo", O_CREAT, S_IRWXG); }
> -mike
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>