[PATCH] src/process.c: remove unnecessary setters

[PATCH] src/process.c: remove unnecessary setters

[Robert Cochran]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

Greetings emacs-devel,

Was looking to get my feet wet for a long-term goal of trying to clean
up existing code, primarily within the C portions of Emacs.

That being said, something that looked small and easy enough to do was
removing some unnecessary functions that only set a struct's member
field. While a compiler can optimize the extra function call away by
inlining it, I think it makes it clearer to the human reader just to set
the struct member directly.

I've run the non-expensive test suite, and all tests not skipped passed
successfully. If there are other ways I should exercise this patch,
please mention them.

Patch is below. Suggestions and comments welcome.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch to remove unncessary statics --]
[-- Type: text/x-patch, Size: 18232 bytes --]

From 7a0fe56d36474a315709673f9a797749f4bd4dab Mon Sep 17 00:00:00 2001
From: Robert Cochran <robert-git@cochranmail.com>
Date: Sat, 27 May 2017 21:04:10 -0700
Subject: [PATCH] ; * src/process.c: remove unnecessary pset_* setters

These functions comprise of simple struct assignments, so just do it
instead of calling a function.

* pset_buffer,
  pset_command,
  pset_decode_coding_system,
  pset_decoding_buf,
  pset_encode_coding_system,
  pset_encoding_buf,
  pset_log,
  pset_mark,
  pset_thread,
  pset_name,
  pset_plist,
  pset_tty_name,
  pset_type,
  pset_write_queue,
  pset_stderrproc: Remove.
* make_process,
  update_processes_for_thread_death,
  set-process-filter,
  set-process-thread,
  set-process-plist,
  make-process,
  create_process,
  create_pty,
  make-pipe-process,
  make-serial-process,
  set_network_socket_coding_system,
  make-network-process,
  server_accept_connection,
  read_and_dispose_of_process_output,
  write_queue_push,
  write_queue_pop,
  send_process,
  stop-process,
  continue-process,
  set-process-coding-system,
  set-process-filter-multibyte: Don't use afforementioned setter
  functions; set struct member directly.
---
 src/process.c | 199 ++++++++++++++++++----------------------------------------
 1 file changed, 61 insertions(+), 138 deletions(-)

diff --git a/src/process.c b/src/process.c
index 2a1c2ee..97f0206 100644
--- a/src/process.c
+++ b/src/process.c
@@ -314,90 +314,15 @@ static struct sockaddr_and_len {
 
 /* These setters are used only in this file, so they can be private.  */
 static void
-pset_buffer (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->buffer = val;
-}
-static void
-pset_command (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->command = val;
-}
-static void
-pset_decode_coding_system (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->decode_coding_system = val;
-}
-static void
-pset_decoding_buf (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->decoding_buf = val;
-}
-static void
-pset_encode_coding_system (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->encode_coding_system = val;
-}
-static void
-pset_encoding_buf (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->encoding_buf = val;
-}
-static void
 pset_filter (struct Lisp_Process *p, Lisp_Object val)
 {
   p->filter = NILP (val) ? Qinternal_default_process_filter : val;
 }
 static void
-pset_log (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->log = val;
-}
-static void
-pset_mark (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->mark = val;
-}
-static void
-pset_thread (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->thread = val;
-}
-static void
-pset_name (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->name = val;
-}
-static void
-pset_plist (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->plist = val;
-}
-static void
 pset_sentinel (struct Lisp_Process *p, Lisp_Object val)
 {
   p->sentinel = NILP (val) ? Qinternal_default_process_sentinel : val;
 }
-static void
-pset_tty_name (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->tty_name = val;
-}
-static void
-pset_type (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->type = val;
-}
-static void
-pset_write_queue (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->write_queue = val;
-}
-static void
-pset_stderrproc (struct Lisp_Process *p, Lisp_Object val)
-{
-  p->stderrproc = val;
-}
 
 \f
 static Lisp_Object
@@ -852,8 +777,8 @@ make_process (Lisp_Object name)
   /* Initialize Lisp data.  Note that allocate_process initializes all
      Lisp data to nil, so do it only for slots which should not be nil.  */
   pset_status (p, Qrun);
-  pset_mark (p, Fmake_marker ());
-  pset_thread (p, Fcurrent_thread ());
+  p->mark = Fmake_marker ();
+  p->thread = Fcurrent_thread ();
 
   /* Initialize non-Lisp data.  Note that allocate_process zeroes out all
      non-Lisp data, so do it only for slots which should not be zero.  */
@@ -882,7 +807,7 @@ make_process (Lisp_Object name)
       name1 = concat2 (name, lsuffix);
     }
   name = name1;
-  pset_name (p, name);
+  p->name = name;
   pset_sentinel (p, Qinternal_default_process_sentinel);
   pset_filter (p, Qinternal_default_process_filter);
   Lisp_Object val;
@@ -914,7 +839,7 @@ update_processes_for_thread_death (Lisp_Object dying_thread)
 	{
 	  struct Lisp_Process *proc = XPROCESS (process);
 
-	  pset_thread (proc, Qnil);
+	  proc->thread = Qnil;
 	  if (proc->infd >= 0)
 	    fd_callback_info[proc->infd].thread = NULL;
 	  if (proc->outfd >= 0)
@@ -1195,7 +1120,7 @@ Return BUFFER.  */)
   if (!NILP (buffer))
     CHECK_BUFFER (buffer);
   p = XPROCESS (process);
-  pset_buffer (p, buffer);
+  p->buffer = buffer;
   if (NETCONN1_P (p) || SERIALCONN1_P (p) || PIPECONN1_P (p))
     pset_childp (p, Fplist_put (p->childp, QCbuffer, buffer));
   setup_process_coding_systems (process);
@@ -1335,7 +1260,7 @@ If THREAD is nil, the process is unlocked.  */)
     }
 
   proc = XPROCESS (process);
-  pset_thread (proc, thread);
+  proc->thread = thread;
   if (proc->infd >= 0)
     fd_callback_info[proc->infd].thread = tstate;
   if (proc->outfd >= 0)
@@ -1491,7 +1416,7 @@ DEFUN ("set-process-plist", Fset_process_plist, Sset_process_plist,
   CHECK_PROCESS (process);
   CHECK_LIST (plist);
 
-  pset_plist (XPROCESS (process), plist);
+  XPROCESS (process)->plist = plist;
   return plist;
 }
 
@@ -1705,16 +1630,16 @@ usage: (make-process &rest ARGS)  */)
 
   pset_childp (XPROCESS (proc), Qt);
   eassert (NILP (XPROCESS (proc)->plist));
-  pset_type (XPROCESS (proc), Qreal);
-  pset_buffer (XPROCESS (proc), buffer);
+  XPROCESS (proc)->type = Qreal;
+  XPROCESS (proc)->buffer = buffer;
   pset_sentinel (XPROCESS (proc), Fplist_get (contact, QCsentinel));
   pset_filter (XPROCESS (proc), Fplist_get (contact, QCfilter));
-  pset_command (XPROCESS (proc), Fcopy_sequence (command));
+  XPROCESS (proc)->command = Fcopy_sequence (command);
 
   if (tem = Fplist_get (contact, QCnoquery), !NILP (tem))
     XPROCESS (proc)->kill_without_query = 1;
   if (tem = Fplist_get (contact, QCstop), !NILP (tem))
-    pset_command (XPROCESS (proc), Qt);
+    XPROCESS (proc)->command = Qt;
 
   tem = Fplist_get (contact, QCconnection_type);
   if (EQ (tem, Qpty))
@@ -1728,7 +1653,7 @@ usage: (make-process &rest ARGS)  */)
 
   if (!NILP (stderrproc))
     {
-      pset_stderrproc (XPROCESS (proc), stderrproc);
+      XPROCESS (proc)->stderrproc = stderrproc;
 
       XPROCESS (proc)->pty_flag = false;
     }
@@ -1788,7 +1713,7 @@ usage: (make-process &rest ARGS)  */)
 	else if (CONSP (Vdefault_process_coding_system))
 	  val = XCAR (Vdefault_process_coding_system);
       }
-    pset_decode_coding_system (XPROCESS (proc), val);
+    XPROCESS (proc)->decode_coding_system = val;
 
     if (!NILP (tem))
       {
@@ -1819,7 +1744,7 @@ usage: (make-process &rest ARGS)  */)
 	else if (CONSP (Vdefault_process_coding_system))
 	  val = XCDR (Vdefault_process_coding_system);
       }
-    pset_encode_coding_system (XPROCESS (proc), val);
+    XPROCESS (proc)->encode_coding_system = val;
     /* Note: At this moment, the above coding system may leave
        text-conversion or eol-conversion unspecified.  They will be
        decided after we read output from the process and decode it by
@@ -1828,9 +1753,9 @@ usage: (make-process &rest ARGS)  */)
   }
 
 
-  pset_decoding_buf (XPROCESS (proc), empty_unibyte_string);
+  XPROCESS (proc)->decoding_buf = empty_unibyte_string;
   eassert (XPROCESS (proc)->decoding_carryover == 0);
-  pset_encoding_buf (XPROCESS (proc), empty_unibyte_string);
+  XPROCESS (proc)->encoding_buf = empty_unibyte_string;
 
   XPROCESS (proc)->inherit_coding_system_flag
     = !(NILP (buffer) || !inherit_process_coding_system);
@@ -2210,7 +2135,7 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
       register_child (pid, inchannel);
 #endif /* WINDOWSNT */
 
-      pset_tty_name (p, lisp_pty_name);
+      p->tty_name = lisp_pty_name;
 
 #ifndef WINDOWSNT
       /* Wait for child_setup to complete in case that vfork is
@@ -2280,7 +2205,7 @@ create_pty (Lisp_Object process)
 
       add_process_read_fd (pty_fd);
 
-      pset_tty_name (p, build_string (pty_name));
+      p->tty_name = build_string (pty_name);
     }
 
   p->pid = -2;
@@ -2369,18 +2294,18 @@ usage:  (make-pipe-process &rest ARGS)  */)
   if (NILP (buffer))
     buffer = name;
   buffer = Fget_buffer_create (buffer);
-  pset_buffer (p, buffer);
+  p->buffer = buffer;
 
   pset_childp (p, contact);
-  pset_plist (p, Fcopy_sequence (Fplist_get (contact, QCplist)));
-  pset_type (p, Qpipe);
+  p->plist = Fcopy_sequence (Fplist_get (contact, QCplist));
+  p->type = Qpipe;
   pset_sentinel (p, Fplist_get (contact, QCsentinel));
   pset_filter (p, Fplist_get (contact, QCfilter));
   eassert (NILP (p->log));
   if (tem = Fplist_get (contact, QCnoquery), !NILP (tem))
     p->kill_without_query = 1;
   if (tem = Fplist_get (contact, QCstop), !NILP (tem))
-    pset_command (p, Qt);
+    p->command = Qt;
   eassert (! p->pty_flag);
 
   if (!EQ (p->command, Qt))
@@ -2428,7 +2353,7 @@ usage:  (make-pipe-process &rest ARGS)  */)
 	else
 	  val = Qnil;
       }
-    pset_decode_coding_system (p, val);
+    p->decode_coding_system = val;
 
     if (!NILP (tem))
       {
@@ -2449,7 +2374,7 @@ usage:  (make-pipe-process &rest ARGS)  */)
 	else
 	  val = Qnil;
       }
-    pset_encode_coding_system (p, val);
+    p->encode_coding_system = val;
   }
   /* This may signal an error.  */
   setup_process_coding_systems (proc);
@@ -3105,18 +3030,18 @@ usage:  (make-serial-process &rest ARGS)  */)
   if (NILP (buffer))
     buffer = name;
   buffer = Fget_buffer_create (buffer);
-  pset_buffer (p, buffer);
+  p->buffer = buffer;
 
   pset_childp (p, contact);
-  pset_plist (p, Fcopy_sequence (Fplist_get (contact, QCplist)));
-  pset_type (p, Qserial);
+  p->plist = Fcopy_sequence (Fplist_get (contact, QCplist));
+  p->type = Qserial;
   pset_sentinel (p, Fplist_get (contact, QCsentinel));
   pset_filter (p, Fplist_get (contact, QCfilter));
   eassert (NILP (p->log));
   if (tem = Fplist_get (contact, QCnoquery), !NILP (tem))
     p->kill_without_query = 1;
   if (tem = Fplist_get (contact, QCstop), !NILP (tem))
-    pset_command (p, Qt);
+    p->command = Qt;
   eassert (! p->pty_flag);
 
   if (!EQ (p->command, Qt))
@@ -3145,7 +3070,7 @@ usage:  (make-serial-process &rest ARGS)  */)
   else if ((!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
 	   || (NILP (buffer) && NILP (BVAR (&buffer_defaults, enable_multibyte_characters))))
     val = Qnil;
-  pset_decode_coding_system (p, val);
+  p->decode_coding_system = val;
 
   val = Qnil;
   if (!NILP (tem))
@@ -3159,12 +3084,12 @@ usage:  (make-serial-process &rest ARGS)  */)
   else if ((!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
 	   || (NILP (buffer) && NILP (BVAR (&buffer_defaults, enable_multibyte_characters))))
     val = Qnil;
-  pset_encode_coding_system (p, val);
+  p->encode_coding_system = val;
 
   setup_process_coding_systems (proc);
-  pset_decoding_buf (p, empty_unibyte_string);
+  p->decoding_buf = empty_unibyte_string;
   eassert (p->decoding_carryover == 0);
-  pset_encoding_buf (p, empty_unibyte_string);
+  p->encoding_buf = empty_unibyte_string;
   p->inherit_coding_system_flag
     = !(!NILP (tem) || NILP (buffer) || !inherit_process_coding_system);
 
@@ -3224,7 +3149,7 @@ set_network_socket_coding_system (Lisp_Object proc, Lisp_Object host,
       else
 	val = Qnil;
     }
-  pset_decode_coding_system (p, val);
+  p->decode_coding_system = val;
 
   if (!NILP (tem))
     {
@@ -3254,11 +3179,11 @@ set_network_socket_coding_system (Lisp_Object proc, Lisp_Object host,
       else
 	val = Qnil;
     }
-  pset_encode_coding_system (p, val);
+  p->encode_coding_system = val;
 
-  pset_decoding_buf (p, empty_unibyte_string);
+  p->decoding_buf = empty_unibyte_string;
   p->decoding_carryover = 0;
-  pset_encoding_buf (p, empty_unibyte_string);
+  p->encoding_buf = empty_unibyte_string;
 
   p->inherit_coding_system_flag
     = !(!NILP (tem) || NILP (p->buffer) || !inherit_process_coding_system);
@@ -4116,17 +4041,17 @@ usage: (make-network-process &rest ARGS)  */)
   record_unwind_protect (remove_process, proc);
   p = XPROCESS (proc);
   pset_childp (p, contact);
-  pset_plist (p, Fcopy_sequence (Fplist_get (contact, QCplist)));
-  pset_type (p, Qnetwork);
+  p->plist = Fcopy_sequence (Fplist_get (contact, QCplist));
+  p->type = Qnetwork;
 
-  pset_buffer (p, buffer);
+  p->buffer = buffer;
   pset_sentinel (p, sentinel);
   pset_filter (p, filter);
-  pset_log (p, Fplist_get (contact, QClog));
+  p->log = Fplist_get (contact, QClog);
   if (tem = Fplist_get (contact, QCnoquery), !NILP (tem))
     p->kill_without_query = 1;
   if ((tem = Fplist_get (contact, QCstop), !NILP (tem)))
-    pset_command (p, Qt);
+    p->command = Qt;
   eassert (p->pid == 0);
   p->backlog = 5;
   eassert (! p->is_non_blocking_client);
@@ -4797,10 +4722,10 @@ server_accept_connection (Lisp_Object server, int channel)
 #endif
 
   pset_childp (p, contact);
-  pset_plist (p, Fcopy_sequence (ps->plist));
-  pset_type (p, Qnetwork);
+  p->plist = Fcopy_sequence (ps->plist);
+  p->type = Qnetwork;
 
-  pset_buffer (p, buffer);
+  p->buffer = buffer;
   pset_sentinel (p, ps->sentinel);
   pset_filter (p, ps->filter);
   eassert (NILP (p->command));
@@ -4825,13 +4750,13 @@ server_accept_connection (Lisp_Object server, int channel)
      of the new process should reflect the settings at the time the
      server socket was opened; not the current settings.  */
 
-  pset_decode_coding_system (p, ps->decode_coding_system);
-  pset_encode_coding_system (p, ps->encode_coding_system);
+  p->decode_coding_system = ps->decode_coding_system;
+  p->encode_coding_system = ps->encode_coding_system;
   setup_process_coding_systems (proc);
 
-  pset_decoding_buf (p, empty_unibyte_string);
+  p->decoding_buf = empty_unibyte_string;
   eassert (p->decoding_carryover == 0);
-  pset_encoding_buf (p, empty_unibyte_string);
+  p->encoding_buf = empty_unibyte_string;
 
   p->inherit_coding_system_flag
     = (NILP (buffer) ? 0 : ps->inherit_coding_system_flag);
@@ -5957,7 +5882,7 @@ read_and_dispose_of_process_output (struct Lisp_Process *p, char *chars,
   /* A new coding system might be found.  */
   if (!EQ (p->decode_coding_system, Vlast_coding_system_used))
     {
-      pset_decode_coding_system (p, Vlast_coding_system_used);
+      p->decode_coding_system = Vlast_coding_system_used;
 
       /* Don't call setup_coding_system for
 	 proc_decode_coding_system[channel] here.  It is done in
@@ -5973,8 +5898,8 @@ read_and_dispose_of_process_output (struct Lisp_Process *p, char *chars,
       if (NILP (p->encode_coding_system) && p->outfd >= 0
 	  && proc_encode_coding_system[p->outfd])
 	{
-	  pset_encode_coding_system
-	    (p, coding_inherit_eol_type (Vlast_coding_system_used, Qnil));
+	  p->encode_coding_system =
+	    coding_inherit_eol_type (Vlast_coding_system_used, Qnil);
 	  setup_coding_system (p->encode_coding_system,
 			       proc_encode_coding_system[p->outfd]);
 	}
@@ -5983,7 +5908,7 @@ read_and_dispose_of_process_output (struct Lisp_Process *p, char *chars,
   if (coding->carryover_bytes > 0)
     {
       if (SCHARS (p->decoding_buf) < coding->carryover_bytes)
-	pset_decoding_buf (p, make_uninit_string (coding->carryover_bytes));
+	p->decoding_buf = make_uninit_string (coding->carryover_bytes);
       memcpy (SDATA (p->decoding_buf), coding->carryover,
 	      coding->carryover_bytes);
       p->decoding_carryover = coding->carryover_bytes;
@@ -6158,9 +6083,9 @@ write_queue_push (struct Lisp_Process *p, Lisp_Object input_obj,
   entry = Fcons (obj, Fcons (make_number (offset), make_number (len)));
 
   if (front)
-    pset_write_queue (p, Fcons (entry, p->write_queue));
+    p->write_queue = Fcons (entry, p->write_queue);
   else
-    pset_write_queue (p, nconc2 (p->write_queue, list1 (entry)));
+    p->write_queue = nconc2 (p->write_queue, list1 (entry));
 }
 
 /* Remove the first element in the write_queue of process P, put its
@@ -6178,7 +6103,7 @@ write_queue_pop (struct Lisp_Process *p, Lisp_Object *obj,
     return 0;
 
   entry = XCAR (p->write_queue);
-  pset_write_queue (p, XCDR (p->write_queue));
+  p->write_queue = XCDR (p->write_queue);
 
   *obj = XCAR (entry);
   offset_length = XCDR (entry);
@@ -6229,8 +6154,7 @@ send_process (Lisp_Object proc, const char *buf, ptrdiff_t len,
 	  && !NILP (BVAR (XBUFFER (object), enable_multibyte_characters)))
       || EQ (object, Qt))
     {
-      pset_encode_coding_system
-	(p, complement_process_encoding_system (p->encode_coding_system));
+      p->encode_coding_system = complement_process_encoding_system (p->encode_coding_system);
       if (!EQ (Vlast_coding_system_used, p->encode_coding_system))
 	{
 	  /* The coding system for encoding was changed to raw-text
@@ -6730,7 +6654,7 @@ of incoming traffic.  */)
       if (NILP (p->command)
 	  && p->infd >= 0)
 	delete_read_fd (p->infd);
-      pset_command (p, Qt);
+      p->command = Qt;
       return process;
     }
 #ifndef SIGTSTP
@@ -6766,7 +6690,7 @@ traffic.  */)
 	  tcflush (p->infd, TCIFLUSH);
 #endif /* not WINDOWSNT */
 	}
-      pset_command (p, Qnil);
+      p->command = Qnil;
       return process;
     }
 #ifdef SIGCONT
@@ -7334,8 +7258,8 @@ encode subprocess input. */)
   Fcheck_coding_system (decoding);
   Fcheck_coding_system (encoding);
   encoding = coding_inherit_eol_type (encoding, Qnil);
-  pset_decode_coding_system (p, decoding);
-  pset_encode_coding_system (p, encoding);
+  p->decode_coding_system = decoding;
+  p->encode_coding_system = encoding;
 
   /* If the sockets haven't been set up yet, the final setup part of
      this will be called asynchronously. */
@@ -7370,8 +7294,7 @@ suppressed.  */)
 
   struct Lisp_Process *p = XPROCESS (process);
   if (NILP (flag))
-    pset_decode_coding_system
-      (p, raw_text_coding_system (p->decode_coding_system));
+    p->decode_coding_system = raw_text_coding_system (p->decode_coding_system);
 
   /* If the sockets haven't been set up yet, the final setup part of
      this will be called asynchronously. */
-- 
2.9.4


[-- Attachment #3: Type: text/plain, Size: 91 bytes --]


-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

Re: [PATCH] src/process.c: remove unnecessary setters

[Paul Eggert]

As I recall those setters and getters were put in for a reason. Have you 
consulted the development history and emacs-devel logs to see why, and/or 
contacted whoever added that code?

Re: [PATCH] src/process.c: remove unnecessary setters

[Robert Cochran]

Paul Eggert <eggert@cs.ucla.edu> writes:

> As I recall those setters and getters were put in for a reason. Have you
> consulted the development history and emacs-devel logs to see why, and/or
> contacted whoever added that code?

Not anymore, based on what I see.

With some git-blame history digging, here's what I found:

Dmitry Antipov added the ancestor PVAR macro in 3193acd2 "Use
INTERNAL_FIELD for processes." on 08/01/2012, and made the struct fields
use the INTERNAL_FIELD macro in the same commit.

This was split into two macros PGET and PSET in 21238f11 "Separate read
and write access to Lisp_Object slots of Lisp_Process." on 08/06/2012.

On 08/07/2012, he entirely drops PGET and removes the use of
INTERNAL_FIELD in 4d2b044c "Drop PGET and revert read access to
Lisp_Objects slots of Lisp_Process".

Then it was you that removed PSET in favor of individual setters in
6a09a33b "* process.h (PSET): Remove.", where the code has sat mostly
untouched since 08/18/2012 aside from being de-inlined by you in
b0ab8123d "Prefer plain 'static' to 'static inline'." on 09/30/2012.

Especially given that in 2015, INTERNAL_FIELD was removed (according to
ChangeLog.2 at any rate), it looks like the reason for the functions
removed to be abstracted away has long since become irrelevant.

I could be wrong though. I'm wrong a lot.

--
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

Re: [PATCH] src/process.c: remove unnecessary setters

[Paul Eggert]

Robert Cochran wrote:
> Especially given that in 2015, INTERNAL_FIELD was removed (according to
> ChangeLog.2 at any rate), it looks like the reason for the functions
> removed to be abstracted away has long since become irrelevant.

Thanks for looking into this. I had forgotten the details. I assume nobody needs 
those getters and setters any more, although it'd be nice to hear from Dmitry. 
I'll CC: him.

Re: [PATCH] src/process.c: remove unnecessary setters

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 29 May 2017 22:19:32 -0700
> Cc: Dmitry Antipov <dmantipov@yandex.ru>, emacs-devel@gnu.org
> 
> Robert Cochran wrote:
> > Especially given that in 2015, INTERNAL_FIELD was removed (according to
> > ChangeLog.2 at any rate), it looks like the reason for the functions
> > removed to be abstracted away has long since become irrelevant.
> 
> Thanks for looking into this. I had forgotten the details. I assume nobody needs 
> those getters and setters any more, although it'd be nice to hear from Dmitry. 
> I'll CC: him.

AFAIR, the idea was to help migrate Emacs to a better GC
implementation.  Not sure if that plan is still on Dmitry's or
someone's todo, or whether we want this to be easier in the future.

We have similar functions for other Lisp objects, for similar reasons.

Re: [PATCH] src/process.c: remove unnecessary setters

[Robert Cochran]

I've let this sit for quite awhile, so I'm checking in on this again.

The patch still applies to master HEAD, so nothing seems to have changed
very much in between now and when I created the patch.

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

Re: [PATCH] src/process.c: remove unnecessary setters

[Paul Eggert]

Could you please remind us again what the patch is? Maybe point to the 
old discussion?

Re: [PATCH] src/process.c: remove unnecessary setters

[Robert Cochran]

Here is the link to the head of this thread in the emacs-devel archive:

https://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00857.html

src/process.c contains a few functions that are essentially just setters
that take a struct Lisp_Process and a Lisp_Object and sets a particular
structure member to the Lisp_Object. An example of one that I removed:

#+BEGIN_SRC C
static void
pset_command (struct Lisp_Process *p, Lisp_Object val)
{
  p->command = val;
}
#+END_SRC

IMO, there's not much of a point in wrapping something so simple in a
funcall. I understand that a good compiler will optimize that away, but
that doesn't really fix the code any.

Whatever reason for leaving these has apparently faded into history, if
my past self is to be believed:

>Paul Eggert <address@hidden> writes:
>
>> As I recall those setters and getters were put in for a reason. Have you
>> consulted the development history and emacs-devel logs to see why, and/or
>> contacted whoever added that code?
>
> Not anymore, based on what I see.
>
> With some git-blame history digging, here's what I found:
>
> Dmitry Antipov added the ancestor PVAR macro in 3193acd2 "Use
> INTERNAL_FIELD for processes." on 08/01/2012, and made the struct fields
> use the INTERNAL_FIELD macro in the same commit.
>
> This was split into two macros PGET and PSET in 21238f11 "Separate read
> and write access to Lisp_Object slots of Lisp_Process." on 08/06/2012.
>
> On 08/07/2012, he entirely drops PGET and removes the use of
> INTERNAL_FIELD in 4d2b044c "Drop PGET and revert read access to
> Lisp_Objects slots of Lisp_Process".
>
> Then it was you that removed PSET in favor of individual setters in
> 6a09a33b "* process.h (PSET): Remove.", where the code has sat mostly
> untouched since 08/18/2012 aside from being de-inlined by you in
> b0ab8123d "Prefer plain 'static' to 'static inline'." on 09/30/2012.
>
> Especially given that in 2015, INTERNAL_FIELD was removed (according to
> ChangeLog.2 at any rate), it looks like the reason for the functions
> removed to be abstracted away has long since become irrelevant.

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

Re: [PATCH] src/process.c: remove unnecessary setters

[Paul Eggert]

Robert Cochran wrote:
> Here is the link to the head of this thread in the emacs-devel archive:
> 
> https://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00857.html

Thanks for reminding us. As Dmitry hasn't spoken up about this for several 
months, I'm inclined to install that simplification patch in master. Let's give 
people a few more days to comment, though.

Re: [PATCH] src/process.c: remove unnecessary setters

[Tom Tromey]

>>>>> "Robert" == Robert Cochran <robert-emacs@cochranmail.com> writes:

Robert> IMO, there's not much of a point in wrapping something so simple in a
Robert> funcall. I understand that a good compiler will optimize that away, but
Robert> that doesn't really fix the code any.

Robert> Whatever reason for leaving these has apparently faded into history, if
Robert> my past self is to be believed:

IIRC those were introduced to support incremental GC.
And, there was an incremental GC patch that followed, though I don't
recall what happened to it.

Tom

Re: [PATCH] src/process.c: remove unnecessary setters

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  Eli Zaretskii <eliz@gnu.org>,  dmantipov@yandex.ru,  emacs-devel@gnu.org
> Date: Fri, 05 Jan 2018 10:58:19 -0700
> 
> >>>>> "Robert" == Robert Cochran <robert-emacs@cochranmail.com> writes:
> 
> Robert> IMO, there's not much of a point in wrapping something so simple in a
> Robert> funcall. I understand that a good compiler will optimize that away, but
> Robert> that doesn't really fix the code any.
> 
> Robert> Whatever reason for leaving these has apparently faded into history, if
> Robert> my past self is to be believed:
> 
> IIRC those were introduced to support incremental GC.

Indeed.  In general, we have similar setters for window, frame, and
buffer objects.  Do we want to get rid of all of those?  And if we do,
does that mean we abandon all hope for migrating to a more modern GC?

Re: [PATCH] src/process.c: remove unnecessary setters

[Robert Cochran]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tom Tromey <tom@tromey.com>
>> Cc: Paul Eggert <eggert@cs.ucla.edu>,  Eli Zaretskii <eliz@gnu.org>,  dmantipov@yandex.ru,  emacs-devel@gnu.org
>> Date: Fri, 05 Jan 2018 10:58:19 -0700
>> 
>> >>>>> "Robert" == Robert Cochran <robert-emacs@cochranmail.com> writes:
>> 
>> Robert> IMO, there's not much of a point in wrapping something so simple in a
>> Robert> funcall. I understand that a good compiler will optimize that away, but
>> Robert> that doesn't really fix the code any.
>> 
>> Robert> Whatever reason for leaving these has apparently faded into history, if
>> Robert> my past self is to be believed:
>> 
>> IIRC those were introduced to support incremental GC.
>
> Indeed.  In general, we have similar setters for window, frame, and
> buffer objects.  Do we want to get rid of all of those?  And if we do,
> does that mean we abandon all hope for migrating to a more modern GC?

If the setters in question are tiny wrappers whose entire body is a
straight assignment to a struct member (like all of the ones I removed
with this patch), then IMO yes, they should go away.

I left both pset_filter() and pset_sentinel() because they had a default
value that was assigned if the value to be assigned was nil. Those
setters have at least a some value and make sense because there is
logic in them, if only a little bit.

But I see absolutely no value in setters that look like the ones I
removed, where the entire body was just an assignment with no logic or
defaults or anything special.

If these kinds of setters become necessary, for doing the GC bookkeeping
that you mention for instance, then by all means put them back once they
do something other that merely set a structure memeber to exactly what
was passed as a value and nothing more.

My intent is that relatively small changes like this help make the C
parts of Emacs less intimidating. I've noticed a general social
perception that the C parts are intimidating, that people generally
don't want to touch it, and that things are getting to a point where
there are less and less people who understand the C parts.

I feel like the appropriate response then, is to find places like this
where some of the accidental complexity can be made to go away. This
hopefully makes it easier to read the code, thus hopefully making it
easier to understand, thus hopefully inspiring confidence to maintain
and extend the C portions of Emacs.

I do not intend for patches of this nature to obstruct future plans for
shiny features such as the aforementioned advanced GC. And if it turns
out in the future that we /need/ to have these setters, then we can put
them back when the time comes.

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - BD0C 5F8B 381C 64F0 F3CE  E7B9 EC9A 872C 41B2 77C2

Re: [PATCH] src/process.c: remove unnecessary setters

[Eli Zaretskii]

> From: Robert Cochran <robert+Emacs@cochranmail.com>
> Cc: Tom Tromey <tom@tromey.com>,  eggert@cs.ucla.edu,  robert-emacs@cochranmail.com,  dmantipov@yandex.ru,  emacs-devel@gnu.org
> Date: Sat, 06 Jan 2018 01:37:41 -0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Tom Tromey <tom@tromey.com>
> >> Cc: Paul Eggert <eggert@cs.ucla.edu>,  Eli Zaretskii <eliz@gnu.org>,  dmantipov@yandex.ru,  emacs-devel@gnu.org
> >> Date: Fri, 05 Jan 2018 10:58:19 -0700
> >> 
> >> >>>>> "Robert" == Robert Cochran <robert-emacs@cochranmail.com> writes:
> >> 
> >> Robert> IMO, there's not much of a point in wrapping something so simple in a
> >> Robert> funcall. I understand that a good compiler will optimize that away, but
> >> Robert> that doesn't really fix the code any.
> >> 
> >> Robert> Whatever reason for leaving these has apparently faded into history, if
> >> Robert> my past self is to be believed:
> >> 
> >> IIRC those were introduced to support incremental GC.
> >
> > Indeed.  In general, we have similar setters for window, frame, and
> > buffer objects.  Do we want to get rid of all of those?  And if we do,
> > does that mean we abandon all hope for migrating to a more modern GC?
> 
> If the setters in question are tiny wrappers whose entire body is a
> straight assignment to a struct member (like all of the ones I removed
> with this patch), then IMO yes, they should go away.

The value is that these setters allow a much easier switch to a more
modern GC, because most such modern GC methods want to know when an
object was modified.  By removing these setters we in effect say we
don't envision any move to a better GC any time soon.

IOW, a decision to remove those setters need to consider more than
just how thick they are today.  It took a non-trivial amount of effort
at the time to come up with just the right mix of setters, and we've
kept them ever since in the hope that they will be useful to someone
who'd want to work on a better GC for Emacs.

> If these kinds of setters become necessary, for doing the GC bookkeeping
> that you mention for instance, then by all means put them back once they
> do something other that merely set a structure memeber to exactly what
> was passed as a value and nothing more.

Like I said: deciding which setters to put back was a non-trivial job,
which took several iterations.  And who will even remember at that
time that we once had these setters?

I'm not saying I object to the removal of these, just that the
decision at stake is more than what meets the eye.

> My intent is that relatively small changes like this help make the C
> parts of Emacs less intimidating. I've noticed a general social
> perception that the C parts are intimidating, that people generally
> don't want to touch it, and that things are getting to a point where
> there are less and less people who understand the C parts.

My impression is that people find the C parts intimidating because
they are less experienced with C, and therefore feel themselves less
"at home" in a large C program.  And I don't see how a setter whose
name says clearly what it does can possibly intimidate.

But once again, this is a side issue.  If we want to decide that GC
will not change any time soon, I don't object to removing these
setters.  But if we do remove them, then we should also remove the
similar wset_*, fset_*, bset_*, and kset_* functions, and I think we
should remove all of them in the same changeset, to make a future
restoring them easier.

> I feel like the appropriate response then, is to find places like this
> where some of the accidental complexity can be made to go away.

IME, the complexity of the Emacs internals is entirely elsewhere: in
the complex problems the code tries to solve, and in lack of deep
enough background information in the docs/comments that would allow
newcomers to dive fairly quickly into the code and understand what the
code is doing.

So I think the contribution of this change to complexity reduction is
relatively minor.  (But again, I'm not against it.)