Re: [PATCH] Renaming non-X x_* identifiers

[Alex Gramiak]

Eli Zaretskii <address@hidden> writes:

>> The hooks I'm referring to are ones that AFAIU don't make any sense on
>> non-HAVE_WINDOW_SYSTEM, and their calls are (should be) already #ifdef'd
>> out (with non-window-system frames never reaching the call). Including
>> the #ifdefs in the declaration side would allow for Hydra to detect
>> cases where an undefined hook is called, which would mean adding the
>> appropriate #ifdef or an appropriate check around that call.
>
> Can you show the list of those hooks you want to #ifdef?  Maybe I'm
> misinterpreting your suggestion.

I've included a diff at the end of my email.

>> I'm a bit confused by your comment on testing. Didn't you say that it
>> was okay that the code that was under HAVE_WINDOW_SYSTEM didn't test for
>> existence of required HAVE_WINDOW_SYSTEM hooks? Those hooks are the ones
>> I was thinking about wrapping into #ifdefs.
>
> Ah, I see the misunderstanding.  Yes, it would be okay to #ifdef the
> calls to those which are only available on window-systems, but then
> why would we test the other kind of hooks for being non-NULL?

The calls should already be #ifdef'd; it's the declarations that would
be #ifdef'd.

> I thought those which don't need to be tested are available on both
> GUI and TTY frames.

I believe there are (currently) three categories:

(a) Hooks implemented by all backends, which don't need to be tested.

(b) Hooks implemented by only GUI frames, but occurring in branches that
non-GUI frames are used. These have to be tested.

(c) Hooks implemented by only GUI frames, and occurring in branches that
only GUI frames are used, and are in a preprocessor conditional. These
don't have to be (and some currently aren't) tested.

The #ifdefs I'm proposing (around declarations of (c) hooks) would
allow Hydra and terminal-only users to catch code that assumes (c) when
it should be (b). The alternatives would be to test all code in (c)
anyway, or just hope that no one made an error by omitting any checks.

P.S. I noticed that the only caller of buffer_flipping_unblocked_hook is
unblock_buffer_flips in xdisp.c. Should this code be #ifdef'd out to
only platforms that do this buffer flipping (HAVE_X_WINDOWS, I believe)?
I'm not sure how much of a performance impact this code has on
redisplay, but it introduces a record_unwind_protect in
redisplay_preserve_echo_area.

diff --git a/src/frame.h b/src/frame.h
index a780ea6085..479d72f20a 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -1597,7 +1597,7 @@ extern char *x_get_resource_string (const char *, const 
char *);
 extern void x_sync (struct frame *);
 #endif /* HAVE_X_WINDOWS */
 
-#ifndef HAVE_NS
+#if defined (HAVE_X_WINDOWS) || defined (HAVE_NTGUI)
 
 /* Set F's bitmap icon, if specified among F's parameters.  */
 
@@ -1610,7 +1610,7 @@ gui_set_bitmap_icon (struct frame *f)
     FRAME_TERMINAL (f)->set_bitmap_icon_hook (f, XCDR (obj));
 }
 
-#endif /* !HAVE_NS */
+#endif /* HAVE_X_WINDOWS || HAVE_NTGUI */
 #endif /* HAVE_WINDOW_SYSTEM */
 
 INLINE void
diff --git a/src/termhooks.h b/src/termhooks.h
index ab1a701bef..a8a2a453e2 100644
--- a/src/termhooks.h
+++ b/src/termhooks.h
@@ -501,9 +501,10 @@ struct terminal
                               bool makeIndex);
 
   /* Multi-frame and mouse support hooks.  */
-
+#ifdef HAVE_WINDOW_SYSTEM
   void (*query_frame_background_color) (struct frame *f, XColor *bgcolor);
 
+#endif
 #if defined (HAVE_X_WINDOWS) || defined (HAVE_NTGUI)
   /* On frame F, translate pixel colors to RGB values for the NCOLORS
      colors in COLORS.  Use cached information, if available.  */
@@ -532,12 +533,13 @@ struct terminal
                                Lisp_Object *y,
                                Time *);
 
+#ifdef HAVE_WINDOW_SYSTEM
   /* This hook is called to get the focus frame.  */
   Lisp_Object (*get_focus_frame) (struct frame *f);
 
   /* This hook is called to shift frame focus.  */
   void (*focus_frame_hook) (struct frame *f, bool noactivate);
-
+#endif
   /* When a frame's focus redirection is changed, this hook tells the
      window system code to re-decide where to put the highlight.  Under
      X, this means that Emacs lies about where the focus is.  */
@@ -555,15 +557,17 @@ struct terminal
      windows.  */
   void (*frame_raise_lower_hook) (struct frame *f, bool raise_flag);
 
+#ifdef HAVE_WINDOW_SYSTEM
   /* This hook is called to make the frame F visible if VISIBLE is
      true, or invisible otherwise. */
   void (*frame_visible_invisible_hook) (struct frame *f, bool visible);
-
+#endif
   /* If the value of the frame parameter changed, this hook is called.
      For example, if going from fullscreen to not fullscreen this hook
      may do something OS dependent, like extended window manager hints on X11. 
 */
   void (*fullscreen_hook) (struct frame *f);
 
+#ifdef HAVE_WINDOW_SYSTEM
   /* This hook is called to iconify the frame.  */
   void (*iconify_frame_hook) (struct frame *f);
 
@@ -589,14 +593,14 @@ struct terminal
   /* This hook is called to set a new font for the frame.  */
   Lisp_Object (*set_new_font_hook) (struct frame *f, Lisp_Object font_object,
                                     int fontset);
-
+#if defined (HAVE_X_WINDOWS) || defined (HAVE_NTGUI)
   /* This hook is called to set the GUI window icon of F using FILE.  */
   bool (*set_bitmap_icon_hook) (struct frame *f, Lisp_Object file);
 
-
+#endif
   void (*implicit_set_name_hook) (struct frame *f, Lisp_Object arg,
                                   Lisp_Object oldval);
-
+#endif
   /* This hook is called to display menus.  */
   Lisp_Object (*menu_show_hook) (struct frame *f, int x, int y, int menuflags,
                                 Lisp_Object title, const char **error_name);
@@ -718,16 +722,19 @@ struct terminal
   /* Called when a frame's display becomes entirely up to date.  */
   void (*frame_up_to_date_hook) (struct frame *);
 
+#ifdef HAVE_X_WINDOWS
   /* Called when buffer flipping becomes unblocked after having
      previously been blocked.  Redisplay always blocks buffer flips
      while it runs.  */
   void (*buffer_flipping_unblocked_hook) (struct frame *);
-
+#endif
+#ifdef HAVE_WINDOW_SYSTEM
   /* Retrieve the string resource specified by NAME with CLASS from
      database RDB. */
   const char * (*get_string_resource_hook) (void *rdb,
                                             const char *name,
                                             const char *class);
+#endif
 
   /* Called to delete the device-specific portions of a frame that is
      on this terminal device. */