Summary:	ASTERISK-05510: Trivial error, include stdio.h
Reporter:	Jose Pablo Fernandez (pupeno)	Labels:
Date Opened:	2005-11-08 12:28:09.000-0600	Date Closed:	2011-06-07 14:11:59
Priority:	Trivial	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) include_astersk_file.h.patch.txt ( 1) trivial-fix.patch
Description:	I had to apply the trivial attached patch to build Asterisk now.
Comments:	By: BJ Weschke (bweschke) 2005-11-08 12:32:28.000-0600 <kpfleming> basically, every module should include stdio, stdlib, unistd, etc. by default <bweschke> k. is it wrong to put #include <stdio.h> back into file.h rather than all of the code #including file.h? <MikeJ[Laptop]> why not just put them in asterisk.h? <kpfleming> they should _never_ rely on an asterisk header to include them for it <bweschke> oh ... ok.. so that's what broke it.. <MikeJ[Laptop]> ok <bweschke> good enough <kpfleming> no, asterisk headers should not include system headers unless absolutely necessary <kpfleming> otherwise, you can';t control the include order <kpfleming> if someone includes lock.h before file.h, then it's bad By: Tilghman Lesher (tilghman) 2005-11-08 17:25:26.000-0600 Yes, but file.h now has a _dependency_ on stdio.h.  Since the system header includes macro safeguards to prevent it from being included more than once, it is safe to include it. Forcing everybody who includes "asterisk/file.h" to have to also make sure they first include <stdio.h> isn't the way to go.  Modules shouldn't have to include dependencies of headers they include, otherwise we have a complete breakdown of the dependency system. I'd argue this is the 'absolutely necessary' exception noted by kpfleming. By: BJ Weschke (bweschke) 2005-11-08 17:42:36.000-0600 assigned to kpfleming for addt'l feedback on corydon76's note. By: Kevin P. Fleming (kpfleming) 2005-11-08 18:36:32.000-0600 The problem with that method is it introduces an ordering dependency on the asterisk headers... lock.h must be included after stdio.h and before file.h (on some platforms that use mutexs in stdio.h). If we are going to go down that road, we might as well just include everything in the proper order in asterisk.h and be done with it; otherwise people will need to keep track of side effects like this. Personally I'd rather have something not compile on _any_ platform because it doesn't include stdio.h rather than have it only fail on _some_ platforms because of the include order. Is that reasonable? Am I off base here? <G> By: Tilghman Lesher (tilghman) 2005-11-08 19:00:31.000-0600 Okay, at the very least, we should include an #error if the FILE type is not defined, telling the administrator to #include <stdio.h> in their custom file.  If we just let all external modules be broken without including a clue, we're never going to hear the end of it. By: Kevin P. Fleming (kpfleming) 2005-11-08 19:15:32.000-0600 I'm OK with that, but 'FILE' is a typedef, I don't think we can test for its existence using preprocessor macros. By: Jose Pablo Fernandez (pupeno) 2005-11-08 19:23:56.000-0600 Are you suggesting that we'll need to modify some sources just to get Asterisk to compile ? By: Tilghman Lesher (tilghman) 2005-11-08 19:47:00.000-0600 No, we're suggesting a change to tell people compiling external modules what they need to change in their external module to get it to compile. By: Tilghman Lesher (tilghman) 2005-11-08 19:56:38.000-0600 I think we can do: #ifndef stdin #error You need to include stdio.h #endif since stdin is defined to be a macro under C98/C99 and is defined in stdio.h. By: Michael Jerris (mikej) 2005-11-08 21:08:06.000-0600 back to kpflemmings comments, from a portability standpoint, we are probably better off to include the common system headers in asterisk.h in the right order.  This will keep ifdefs for all of that stuff out of the files all over asterisk.  Particularly this can be a generated file going forward if we go to autoconf and can modify order\locatiton of includes as necessary, for now we can properly ifdef for that stuff.  Then, the only requiremnet ends up being that you include asterisk.h as the fist include in each header file and c file, and remove the common system headers from everything else. By: Kevin P. Fleming (kpfleming) 2005-11-08 21:14:16.000-0600 That change is too radical to make at this point in the release cycle; I've committed a fix using Corydon's suggestion for now, which will at least ensure that out-of-tree modules can be easily updated to build against the 1.2 release source code. By: opsys (opsys) 2005-11-09 22:45:46.000-0600 Created new diff file rearanging the the logic of testing after the include instead of before. By: Tilghman Lesher (tilghman) 2005-11-09 23:31:48.000-0600 I think you missed the part where we said that we weren't going to include stdio.h in file.h. By: Digium Subversion (svnbot) 2008-01-15 15:55:09.000-0600 Repository: asterisk Revision: 7041 U   trunk/ChangeLog U   trunk/include/asterisk/file.h ------------------------------------------------------------------------ r7041 | kpfleming | 2008-01-15 15:55:08 -0600 (Tue, 15 Jan 2008) | 2 lines issue ASTERISK-5510, different fix ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=7041