Emacs Wrestling Match: lisp.h vs rest of the includes

Emacs Wrestling Match: lisp.h vs rest of the includes

[Juanma Barranquero]

Now that Dan is doing a wonderful job of cleaning up the sources by
building them under different -W settings to detect problems, I've
been inspired to do the same thing.

Building under -Wredundant-decls generates *a lot* of warnings. Some
of them are difficult to avoid, for example the ones generated by
EXFUN. Others are easier; see the attached patch, for starters. It
fixes quite a few warnings generated by declarations duplicated in
lisp.h and the relevant includes (for example,
redisplay_preserve_echo_area is both in lisp.h and dispextern.h).

The problem with these changes is that they represent moving things in
the opposite direction of what I think would be right: from the
includes to lisp.h (not that I'm moving anything to lisp.h, just
keeping the declaration on lisp.h and removing the other one).

I'm sure there are a lot of historical reasons why lisp.h is such
humongous mass of disparate content (compilation time being, I
suspect, the first and main reason in the past). But now that lisp.h
is listed in the dependencies of all .c sources, that's not so
relevant anymore. I think we should make an effort to *remove* things
from lisp.h and back to where they belong. In some cases it will mean
adding more .h dependencies to some sources, and perhaps just for one,
or a few, external declarations. But it will be cleaner and more
maintenable IMO.

    Juanma




=== modified file 'src/dispextern.h'
--- src/dispextern.h	2010-08-09 09:35:21 +0000
+++ src/dispextern.h	2010-08-09 10:18:06 +0000
@@ -2925,7 +2925,6 @@
 void remember_mouse_glyph (struct frame *, int, int, NativeRectangle *);

 void mark_window_display_accurate (Lisp_Object, int);
-void redisplay_preserve_echo_area (int);
 int set_cursor_from_row (struct window *, struct glyph_row *,
                          struct glyph_matrix *, int, int, int, int);
 void init_iterator (struct it *, struct window *, EMACS_INT,
@@ -2946,7 +2945,6 @@
 int in_display_vector_p (struct it *);
 int frame_mode_line_height (struct frame *);
 void highlight_trailing_whitespace (struct frame *, struct glyph_row *);
-extern Lisp_Object Qtool_bar;
 extern Lisp_Object Vshow_trailing_whitespace;
 extern int mode_line_in_non_selected_windows;
 extern int redisplaying_p;
@@ -3088,7 +3086,6 @@
 void unrequest_sigio (void);
 int tabs_safe_p (int);
 void init_baud_rate (int);
-void init_sigio (int);

 /* Defined in xfaces.c */

@@ -3133,7 +3130,6 @@
 int compute_char_face (struct frame *, int, Lisp_Object);
 void free_all_realized_faces (Lisp_Object);
 void free_realized_face (struct frame *, struct face *);
-extern Lisp_Object Qforeground_color, Qbackground_color;
 extern Lisp_Object Qframe_set_background_mode;
 extern char unspecified_fg[], unspecified_bg[];

@@ -3245,8 +3241,6 @@
 void update_single_window (struct window *, int);
 void do_pending_window_change (int);
 void change_frame_size (struct frame *, int, int, int, int, int);
-void init_display (void);
-void syms_of_display (void);
 extern Lisp_Object Qredisplay_dont_pause;
 void spec_glyph_lookup_face (struct window *, GLYPH *);


=== modified file 'src/frame.h'
--- src/frame.h	2010-08-05 23:15:24 +0000
+++ src/frame.h	2010-08-09 03:05:28 +0000
@@ -825,7 +825,7 @@
        list_var = XCDR (list_var))


-extern Lisp_Object Qframep, Qframe_live_p;
+extern Lisp_Object Qframe_live_p;
 extern Lisp_Object Qtty, Qtty_type;
 extern Lisp_Object Qtty_color_mode;
 extern Lisp_Object Qterminal, Qterminal_live_p;
@@ -854,9 +854,6 @@

 extern Lisp_Object Vmouse_highlight;
 \f
-/* The currently selected frame.  */
-
-extern Lisp_Object selected_frame;

 /* Value is a pointer to the selected frame.  If the selected frame
    isn't live, abort.  */
@@ -1045,7 +1042,6 @@
 extern Lisp_Object Qborder_color, Qborder_width;
 extern Lisp_Object Qbuffer_predicate, Qbuffer_list, Qburied_buffer_list;
 extern Lisp_Object Qcursor_color, Qcursor_type;
-extern Lisp_Object Qfont;
 extern Lisp_Object Qbackground_color, Qforeground_color;
 extern Lisp_Object Qicon, Qicon_name, Qicon_type, Qicon_left, Qicon_top;
 extern Lisp_Object Qinternal_border_width;
@@ -1070,15 +1066,11 @@
 extern Lisp_Object Qheight, Qwidth;
 extern Lisp_Object Qminibuffer, Qmodeline;
 extern Lisp_Object Qx, Qw32, Qmac, Qpc, Qns;
-extern Lisp_Object Qvisible;
 extern Lisp_Object Qdisplay_type;
 extern Lisp_Object Qbackground_mode;

 extern Lisp_Object Qx_resource_name;

-extern Lisp_Object Qleft, Qright, Qtop, Qbox, Qbottom;
-extern Lisp_Object Qdisplay;
-
 extern Lisp_Object Qrun_hook_with_args;

 #ifdef HAVE_WINDOW_SYSTEM
@@ -1096,8 +1088,6 @@

 /* These are in frame.c  */

-extern Lisp_Object Vx_resource_name;
-extern Lisp_Object Vx_resource_class;
 extern Lisp_Object Vmenu_bar_mode, Vtool_bar_mode;



=== modified file 'src/w32.h'
--- src/w32.h	2010-01-13 08:35:10 +0000
+++ src/w32.h	2010-08-09 02:39:33 +0000
@@ -139,7 +139,6 @@
 extern void term_w32select (void);
 extern void syms_of_w32menu (void);
 extern void globals_of_w32menu (void);
-extern void syms_of_fontset (void);

 extern int _sys_read_ahead (int fd);
 extern int _sys_wait_accept (int fd);

Re: Emacs Wrestling Match: lisp.h vs rest of the includes

[Stefan Monnier]

> The problem with these changes is that they represent moving things in
> the opposite direction of what I think would be right: from the
> includes to lisp.h (not that I'm moving anything to lisp.h, just
> keeping the declaration on lisp.h and removing the other one).

Indeed, we generally want to move them to the specific .h that
corresponds to where they're defined.  Why did you choose to keep the
lisp.h version rather than the other?


        Stefan

Re: Emacs Wrestling Match: lisp.h vs rest of the includes

[Juanma Barranquero]

On Tue, Aug 10, 2010 at 10:22, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Indeed, we generally want to move them to the specific .h that
> corresponds to where they're defined.

I'm very glad to hear that.

> Why did you choose to keep the lisp.h version rather than the other?

I haven't seen (or haven't noticed) any tendency towards moving things
out of lisp.h. Otherwise I wouldn't have asked.

(I didn't commit the change anyway, it was just an example.)

    Juanma

Re: Emacs Wrestling Match: lisp.h vs rest of the includes

[Stefan Monnier]

>> Why did you choose to keep the lisp.h version rather than the other?
> I haven't seen (or haven't noticed) any tendency towards moving things
> out of lisp.h.  Otherwise I wouldn't have asked.

Indeed, we haven't made a strong attempt at doing it, but it was decided
a long time ago that we wanted to do that (a bit before I made a bunch
of changes to keymap.c where I took the opportunity to create
a keymap.h and move some of the declarations from lisp.h to this new
file, i.e. late 2001 accoding to "bzr log").


        Stefan

Re: Emacs Wrestling Match: lisp.h vs rest of the includes

[Juanma Barranquero]

On Wed, Aug 11, 2010 at 09:51, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> (a bit before I made a bunch
> of changes to keymap.c where I took the opportunity to create
> a keymap.h and move some of the declarations from lisp.h to this new
> file, i.e. late 2001 accoding to "bzr log").

That's a few months before I landed here. No wonder I don't remember it :-)

    Juanma