Patchwork ObjC/ObjC++: Tidy up #includes of objc-act.c

login
register
mail settings
Submitter Nicola Pero
Date Dec. 1, 2010, 9:53 p.m.
Message ID <1291240399.293516255@192.168.2.227>
Download mbox | patch
Permalink /patch/73913/
State New
Headers show

Comments

Nicola Pero - Dec. 1, 2010, 9:53 p.m.
This patch tidies up the list of #includes in objc-act.c.  For each file,
it adds an explanation of why it is included (as far as I could determine).

A few files were not needed and I could remove the line that #included them
(great).

A few other files are implicitly #included by tree.h or c-tree.h etc. and
I left them, commented out, but with a comment explaining what is happening.

I guess it's kind of a trivial patch, but as I did the cleanup, I'd like
to commit it.

Ok to commit ?

Thanks
Mike Stump - Dec. 1, 2010, 10:45 p.m.
On Dec 1, 2010, at 1:53 PM, Nicola Pero wrote:
> This patch tidies up the list of #includes in objc-act.c.  For each file,
> it adds an explanation of why it is included (as far as I could determine).

No other part of the compiler does this...  I'm not strongly opposed, but I just don't see the point either.

> A few other files are implicitly #included by tree.h or c-tree.h etc. and
> I left them, commented out, but with a comment explaining what is happening.

I don't think this should be done.  We include the headers we need, other headers are free, as an implementation detail to include or not the other file.  As they do, we don't want the header to go missing.

> #include "system.h"
> #include "coretypes.h"
> -#include "tm.h"
> #include "tree.h"

Hum, you didn't actually test this did you?

/* Predefine the following data type:                                            
                                                                                 
   struct _objc_exception_data                                                   
   {                                                                             
     int buf[OBJC_JBLEN];                                                        
     void *pointers[4];                                                          
   }; */

/* The following yuckiness should prevent users from having to #include          
   <setjmp.h> in their code... */

/* Define to a harmless positive value so the below code doesn't die.  */
#ifndef OBJC_JBLEN
#define OBJC_JBLEN 18
#endif


and from i386/darwin.h:

/* Size of the Obj-C jump buffer.  */
#define OBJC_JBLEN ((TARGET_64BIT) ? ((9 * 2) + 3 + 16) : (18))

?
Nicola Pero - Dec. 1, 2010, 10:51 p.m.
> I don't think this should be done.

Ok, no big deal.  We can leave the #includes as they are; presumably
the people who are working on cleaning the compiler includes will clean
them up themselves in a way that is consistent with the rest of the
compiler.

>> #include "system.h"
>> #include "coretypes.h"
>> -#include "tm.h"
>> #include "tree.h"
>>
>> Hum, you didn't actually test this did you?

Of course I did.  It still works because target.h pulls in tm.h anyway.

Thanks
Andrew Pinski - Dec. 1, 2010, 10:54 p.m.
On Wed, Dec 1, 2010 at 2:45 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Dec 1, 2010, at 1:53 PM, Nicola Pero wrote:
>> This patch tidies up the list of #includes in objc-act.c.  For each file,
>> it adds an explanation of why it is included (as far as I could determine).
>
> No other part of the compiler does this...  I'm not strongly opposed, but I just don't see the point either.

Actually some parts of the compiler are getting this.  The reasoning
is that some headers really should not be included but currently are
because of dependencies.

-- Pinski
Nicola Pero - Dec. 1, 2010, 11:45 p.m.
>> This patch tidies up the list of #includes in objc-act.c.  For each file,
>> it adds an explanation of why it is included (as far as I could determine).
>
> No other part of the compiler does this...  I'm not strongly opposed,
> but I just don't see the point either.

By the way, just to clarify, including unused headers is bad for many reasons,
but very practically it slows down compilation and it means that changes in
these headers trigger useless rebuilds.

I particularly hate the useless rebuilds, which happen a lot with GCC.  Since
almost every file includes almost every header, any change in any header will
produce a full 40-minute-or-so 3-stage rebuild.  That is a very practical
problem that I have every time I update one of my subversion checkouts (and
that everyone else developing for GCC must be having too!). ;-)

Of course, my patch is a drop in the ocean and was incomplete in that respect
as I didn't actually review/update the dependencies in Make-lang.in ... :-(

Anyway, just wanted to say that the concept of cleaning up header includes and
documenting why headers are included is good.

And we probably should have more, but smaller, headers, so that you can include
only the bits you need, reducing the number of files that need to be rebuilt
each time a header is changed.

Thanks
Joseph S. Myers - Dec. 2, 2010, 12:05 a.m.
On Wed, 1 Dec 2010, Nicola Pero wrote:

> +/* For warn_deprecated_use(), rest_of_decl_compilation().  */
>  #include "toplev.h"

This comment is out of date.  warn_deprecated_use is now in tree.h.
Joseph S. Myers - Dec. 2, 2010, 12:07 a.m.
On Wed, 1 Dec 2010, Nicola Pero wrote:

> >> #include "system.h"
> >> #include "coretypes.h"
> >> -#include "tm.h"
> >> #include "tree.h"
> >>
> >> Hum, you didn't actually test this did you?
> 
> Of course I did.  It still works because target.h pulls in tm.h anyway.

I really don't recommend relying on that since we'd like to stop target.h 
pulling in tm.h.  Instead, comment the tm.h include with a list of all 
target macros you can find being used in this file.

In general, unless it is clear that a header logically *should* export all 
the interfaces from another header, I recommend directly including all the 
headers from which you use any interface, even if some of them are 
implicitly included by another header as well.
Nicola Pero - Dec. 2, 2010, 12:24 a.m.
> I really don't recommend relying on that since we'd like to stop target.h 
> pulling in tm.h.  Instead, comment the tm.h include with a list of all 
> target macros you can find being used in this file.
>
> In general, unless it is clear that a header logically *should* export all 
> the interfaces from another header, I recommend directly including all the 
> headers from which you use any interface, even if some of them are 
> implicitly included by another header as well.

Thanks Joseph

I'll post an updated patch taking into account your comments (which I think
are also similar to/address Mike's main concerns).  I also want to look at
objcp/, the other files and Makefiles, so we clean them all up.

Thanks
Mike Stump - Dec. 2, 2010, 1:14 a.m.
On Dec 1, 2010, at 3:45 PM, Nicola Pero wrote:
> I particularly hate the useless rebuilds, which happen a lot with GCC.  Since
> almost every file includes almost every header, any change in any header will
> produce a full 40-minute-or-so 3-stage rebuild.  That is a very practical
> problem that I have every time I update one of my subversion checkouts (and
> that everyone else developing for GCC must be having too!). ;-)

ccache?  Add code to gcc to make these sorts of rebuilds fast!  :-)

Patch

Index: objc-act.c
===================================================================
--- objc-act.c  (revision 167353)
+++ objc-act.c  (working copy)
@@ -20,10 +20,10 @@  You should have received a copy of the GNU General
 along with GCC; see the file COPYING3.  If not see
 <http://www.gnu.org/licenses/>.  */
 
+/* Basic includes.  */
 #include "config.h"
 #include "system.h"
 #include "coretypes.h"
-#include "tm.h"
 #include "tree.h"
 
 #ifdef OBJCPLUS
@@ -33,30 +33,49 @@  along with GCC; see the file COPYING3.  If not see
 #include "c-lang.h"
 #endif
 
+/* Declarations of most functions in this file.  */
 #include "c-family/c-common.h"
-#include "c-family/c-pragma.h"
-#include "c-family/c-format.h"
-#include "flags.h"
+
+/* For lang_hooks.  */
 #include "langhooks.h"
+
+/* Header associated with this file.  */
 #include "objc-act.h"
-#include "input.h"
-#include "function.h"
+
+/* For input_location.  Already included by tree.h.  */
+/* #include "input.h" */
+
+/* For assemble_external().  */
 #include "output.h"
+
+/* For warn_deprecated_use(), rest_of_decl_compilation().  */
 #include "toplev.h"
-#include "ggc.h"
+
+/* Already included by gimple.h.  */
+/* #include "ggc.h" */
+
+/* For debug_hooks, do_nothing_debug_hooks.  */
 #include "debug.h"
+
+/* For targetcm.  */
 #include "target.h"
-#include "diagnostic-core.h"
+
+/* For warning(), error(), etc.  Already included by c-tree.h or cp-tree.h.  */
+/* #include "diagnostic-core.h" */
+
+/* For _().  */
 #include "intl.h"
+
+/* For cgraph_mark_needed_node(), cgraph_node().  */
 #include "cgraph.h"
+
+/* For tree_stmt_iterator, append_to_statement_list().  */
 #include "tree-iterator.h"
-#include "hashtab.h"
-#include "langhooks-def.h"
 
-/* For default_tree_printer ().  */
-#include "tree-pretty-print.h"
+/* Already included by tree.h.  */
+/* #include "hashtab.h" */
 
-/* For enum gimplify_status */
+/* For enum gimplify_status, gimplify_expr(), is_gimple_val(), etc.  */
 #include "gimple.h"
 
 #define OBJC_VOID_AT_END       void_list_node
Index: ChangeLog
===================================================================
--- ChangeLog   (revision 167353)
+++ ChangeLog   (working copy)
@@ -1,3 +1,10 @@ 
+2010-12-01  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * objc-act.c: Do not include tm.h, c-family/c-pragma.h,
+       c-family/c-format.h, flags.h, input.h, function.h, gcc.h,
+       diagnostic-core.h, hashtab.h, langhooks-def.h and
+       tree-pretty-print.h.  Updated comments for #includes.
+
 2010-11-30  Nicola Pero  <nicola.pero@meta-innovation.com>
 
        * objc-act.c (objc_build_volatilized_type): Removed.