all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Function attributes for make-docfile
@ 2015-01-12  4:09 Dmitry Antipov
  2015-01-12  5:33 ` Stefan Monnier
  2015-01-12  5:49 ` Paul Eggert
  0 siblings, 2 replies; 15+ messages in thread
From: Dmitry Antipov @ 2015-01-12  4:09 UTC (permalink / raw)
  To: Emacs development discussions; +Cc: Paul Eggert

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

The following patch helps make-docfile to generate _Noreturn and ATTRIBUTE_CONST
by looking at special comments found immediately after DEFUN declaration.  This
code works only for regular-form docstrings (doc: /* xxx */), so Flookup_image
is also fixed (the latter should be done anyway, IMO).  Any thoughts?

Dmitry

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: function_attributes.patch --]
[-- Type: text/x-diff; name="function_attributes.patch", Size: 17423 bytes --]

diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index bc5420e..dcc08ea 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -562,6 +562,7 @@ struct global
 {
   enum global_type type;
   char *name;
+  int flags;
   union
   {
     int value;
@@ -569,13 +570,16 @@ struct global
   } v;
 };
 
+/* Bit values for FLAGS field from the above.  */
+enum { NORETURN = 1, CONST = 2 };
+
 /* All the variable names we saw while scanning C sources in `-g'
    mode.  */
 int num_globals;
 int num_globals_allocated;
 struct global *globals;
 
-static void
+static struct global *
 add_global (enum global_type type, char *name, int value, char const *svalue)
 {
   /* Ignore the one non-symbol that can occur.  */
@@ -601,7 +605,10 @@ add_global (enum global_type type, char *name, int value, char const *svalue)
 	globals[num_globals - 1].v.svalue = svalue;
       else
 	globals[num_globals - 1].v.value = value;
+      globals[num_globals - 1].flags = 0;
+      return globals + num_globals - 1;
     }
+  return NULL;
 }
 
 static int
@@ -708,13 +715,7 @@ write_globals (void)
 		globals[i].name, globals[i].name, globals[i].name);
       else
 	{
-	  /* It would be nice to have a cleaner way to deal with these
-	     special hacks.  */
-	  if (strcmp (globals[i].name, "Fthrow") == 0
-	      || strcmp (globals[i].name, "Ftop_level") == 0
-	      || strcmp (globals[i].name, "Fkill_emacs") == 0
-	      || strcmp (globals[i].name, "Fexit_recursive_edit") == 0
-	      || strcmp (globals[i].name, "Fabort_recursive_edit") == 0)
+	  if (globals[i].flags & NORETURN)
 	    fputs ("_Noreturn ", stdout);
 
 	  printf ("EXFUN (%s, ", globals[i].name);
@@ -726,36 +727,7 @@ write_globals (void)
 	    printf ("%d", globals[i].v.value);
 	  putchar (')');
 
-	  /* It would be nice to have a cleaner way to deal with these
-	     special hacks, too.  */
-	  if (strcmp (globals[i].name, "Fatom") == 0
-	      || strcmp (globals[i].name, "Fbyteorder") == 0
-	      || strcmp (globals[i].name, "Fcharacterp") == 0
-	      || strcmp (globals[i].name, "Fchar_or_string_p") == 0
-	      || strcmp (globals[i].name, "Fconsp") == 0
-	      || strcmp (globals[i].name, "Feq") == 0
-	      || strcmp (globals[i].name, "Fface_attribute_relative_p") == 0
-	      || strcmp (globals[i].name, "Fframe_windows_min_size") == 0
-	      || strcmp (globals[i].name, "Fgnutls_errorp") == 0
-	      || strcmp (globals[i].name, "Fidentity") == 0
-	      || strcmp (globals[i].name, "Fintegerp") == 0
-	      || strcmp (globals[i].name, "Finteractive") == 0
-	      || strcmp (globals[i].name, "Ffloatp") == 0
-	      || strcmp (globals[i].name, "Flistp") == 0
-	      || strcmp (globals[i].name, "Fmax_char") == 0
-	      || strcmp (globals[i].name, "Fnatnump") == 0
-	      || strcmp (globals[i].name, "Fnlistp") == 0
-	      || strcmp (globals[i].name, "Fnull") == 0
-	      || strcmp (globals[i].name, "Fnumberp") == 0
-	      || strcmp (globals[i].name, "Fstringp") == 0
-	      || strcmp (globals[i].name, "Fsymbolp") == 0
-	      || strcmp (globals[i].name, "Ftool_bar_height") == 0
-	      || strcmp (globals[i].name, "Fwindow__sanitize_window_sizes") == 0
-#ifndef WINDOWSNT
-	      || strcmp (globals[i].name, "Fgnutls_available_p") == 0
-	      || strcmp (globals[i].name, "Fzlib_available_p") == 0
-#endif
-	      || 0)
+	  if (globals[i].flags & CONST)
 	    fputs (" ATTRIBUTE_CONST", stdout);
 
 	  puts (";");
@@ -1033,7 +1005,70 @@ scan_c_stream (FILE *infile)
 
       if (generate_globals)
 	{
-	  add_global (FUNCTION, name, maxargs, 0);
+	  struct global *g = add_global (FUNCTION, name, maxargs, 0);
+	  /* The following code tries to recognize SPECIAL-COMMENT in:
+
+	     DEFUN ("foo", Ffoo, Sfoo, X, Y, Z,
+	            doc: [docstring]) [SPECIAL-COMMENT]
+	       (Lisp_Object arg...)
+
+	     and use the latter to specify special attributes.
+	     Currently they are _Noreturn and ATTRIBUTE_CONST.  */
+	  c = getc (infile);
+	  if (c == EOF)
+	    goto eof;
+	  int d = getc (infile);
+	  if (d == EOF)
+	    goto eof;
+	  int e = getc (infile);
+	  if (e == EOF)
+	    goto eof;
+	  while (1)
+	    {
+	      if (c == '*' && d == '/' && e == ')')
+		break;
+	      c = d, d = e, e = getc (infile);
+	      if (e == EOF)
+		goto eof;
+	    }
+	  /* Found '*' '/' ')'.  */
+	  do
+	    {
+	      c = getc (infile);
+	      if (c == EOF)
+		goto eof;
+	    }
+	  while (c == ' ' || c == '\n' || c == '\r' || c == '\t');
+	  /* Spaces are skipped.  */
+	  if (c == '/')
+	    {
+	      int d = getc (infile);
+	      if (d == EOF)
+		goto eof;
+	      if (d == '*')
+		{
+		  /* Found start of the comment.  */
+		  char *p = input_buffer;
+		  *p++ = '/', *p++ = '*';
+		  while (1)
+		    {
+		      if (c == '*' && d == '/')
+			break;
+		      c = d, d = getc (infile);
+		      if (d == EOF)
+			goto eof;
+		      *p++ = d;
+		      if (p - input_buffer > sizeof (input_buffer))
+			abort ();
+		    }
+		  *p++ = '\0';
+		  /* The comment is a string in an input buffer.  */
+		  if (strstr (input_buffer, "NORETURN"))
+		    g->flags |= NORETURN;
+		  if (strstr (input_buffer, "CONST"))
+		    g->flags |= CONST;
+		}
+	    }
 	  continue;
 	}
 
diff --git a/src/callint.c b/src/callint.c
index 2595503..b6514df 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -101,7 +101,7 @@ If the string begins with `^' and `shift-select-mode' is non-nil,
  Emacs first calls the function `handle-shift-selection'.
 You may use `@', `*', and `^' together.  They are processed in the
  order that they appear, before reading any arguments.
-usage: (interactive &optional ARGS)  */)
+usage: (interactive &optional ARGS)  */) /* CONST */
   (Lisp_Object args)
 {
   return Qnil;
diff --git a/src/character.c b/src/character.c
index 4a5c7ec..f4c9ea4 100644
--- a/src/character.c
+++ b/src/character.c
@@ -232,14 +232,14 @@ DEFUN ("characterp", Fcharacterp, Scharacterp, 1, 2, 0,
 In Emacs Lisp, characters are represented by character codes, which
 are non-negative integers.  The function `max-char' returns the
 maximum character code.
-usage: (characterp OBJECT)  */)
+usage: (characterp OBJECT)  */) /* CONST */
   (Lisp_Object object, Lisp_Object ignore)
 {
   return (CHARACTERP (object) ? Qt : Qnil);
 }
 
 DEFUN ("max-char", Fmax_char, Smax_char, 0, 0, 0,
-       doc: /* Return the character of the maximum code.  */)
+       doc: /* Return the character of the maximum code.  */) /* CONST */
   (void)
 {
   return make_number (MAX_CHAR);
diff --git a/src/data.c b/src/data.c
index 820c3ce..2f35b56 100644
--- a/src/data.c
+++ b/src/data.c
@@ -177,6 +177,7 @@ args_out_of_range_3 (Lisp_Object a1, Lisp_Object a2, Lisp_Object a3)
 
 DEFUN ("eq", Feq, Seq, 2, 2, 0,
        doc: /* Return t if the two args are the same Lisp object.  */)
+/* CONST */
   (Lisp_Object obj1, Lisp_Object obj2)
 {
   if (EQ (obj1, obj2))
@@ -185,7 +186,7 @@ DEFUN ("eq", Feq, Seq, 2, 2, 0,
 }
 
 DEFUN ("null", Fnull, Snull, 1, 1, 0,
-       doc: /* Return t if OBJECT is nil.  */)
+       doc: /* Return t if OBJECT is nil.  */) /* CONST */
   (Lisp_Object object)
 {
   if (NILP (object))
@@ -263,7 +264,7 @@ for example, (type-of 1) returns `integer'.  */)
 }
 
 DEFUN ("consp", Fconsp, Sconsp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a cons cell.  */)
+       doc: /* Return t if OBJECT is a cons cell.  */) /* CONST */
   (Lisp_Object object)
 {
   if (CONSP (object))
@@ -273,6 +274,7 @@ DEFUN ("consp", Fconsp, Sconsp, 1, 1, 0,
 
 DEFUN ("atom", Fatom, Satom, 1, 1, 0,
        doc: /* Return t if OBJECT is not a cons cell.  This includes nil.  */)
+/* CONST */
   (Lisp_Object object)
 {
   if (CONSP (object))
@@ -282,7 +284,7 @@ DEFUN ("atom", Fatom, Satom, 1, 1, 0,
 
 DEFUN ("listp", Flistp, Slistp, 1, 1, 0,
        doc: /* Return t if OBJECT is a list, that is, a cons cell or nil.
-Otherwise, return nil.  */)
+Otherwise, return nil.  */) /* CONST */
   (Lisp_Object object)
 {
   if (CONSP (object) || NILP (object))
@@ -292,6 +294,7 @@ Otherwise, return nil.  */)
 
 DEFUN ("nlistp", Fnlistp, Snlistp, 1, 1, 0,
        doc: /* Return t if OBJECT is not a list.  Lists include nil.  */)
+/* CONST */
   (Lisp_Object object)
 {
   if (CONSP (object) || NILP (object))
@@ -300,7 +303,7 @@ DEFUN ("nlistp", Fnlistp, Snlistp, 1, 1, 0,
 }
 \f
 DEFUN ("symbolp", Fsymbolp, Ssymbolp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a symbol.  */)
+       doc: /* Return t if OBJECT is a symbol.  */) /* CONST */
   (Lisp_Object object)
 {
   if (SYMBOLP (object))
@@ -333,7 +336,7 @@ DEFUN ("vectorp", Fvectorp, Svectorp, 1, 1, 0,
 }
 
 DEFUN ("stringp", Fstringp, Sstringp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a string.  */)
+       doc: /* Return t if OBJECT is a string.  */) /* CONST */
   (Lisp_Object object)
 {
   if (STRINGP (object))
@@ -436,7 +439,7 @@ DEFUN ("byte-code-function-p", Fbyte_code_function_p, Sbyte_code_function_p,
 }
 
 DEFUN ("char-or-string-p", Fchar_or_string_p, Schar_or_string_p, 1, 1, 0,
-       doc: /* Return t if OBJECT is a character or a string.  */)
+       doc: /* Return t if OBJECT is a character or a string.  */) /* CONST */
   (register Lisp_Object object)
 {
   if (CHARACTERP (object) || STRINGP (object))
@@ -445,7 +448,7 @@ DEFUN ("char-or-string-p", Fchar_or_string_p, Schar_or_string_p, 1, 1, 0,
 }
 \f
 DEFUN ("integerp", Fintegerp, Sintegerp, 1, 1, 0,
-       doc: /* Return t if OBJECT is an integer.  */)
+       doc: /* Return t if OBJECT is an integer.  */) /* CONST */
   (Lisp_Object object)
 {
   if (INTEGERP (object))
@@ -463,7 +466,7 @@ DEFUN ("integer-or-marker-p", Finteger_or_marker_p, Sinteger_or_marker_p, 1, 1,
 }
 
 DEFUN ("natnump", Fnatnump, Snatnump, 1, 1, 0,
-       doc: /* Return t if OBJECT is a nonnegative integer.  */)
+       doc: /* Return t if OBJECT is a nonnegative integer.  */) /* CONST */
   (Lisp_Object object)
 {
   if (NATNUMP (object))
@@ -473,6 +476,7 @@ DEFUN ("natnump", Fnatnump, Snatnump, 1, 1, 0,
 
 DEFUN ("numberp", Fnumberp, Snumberp, 1, 1, 0,
        doc: /* Return t if OBJECT is a number (floating point or integer).  */)
+/* CONST */
   (Lisp_Object object)
 {
   if (NUMBERP (object))
@@ -493,6 +497,7 @@ DEFUN ("number-or-marker-p", Fnumber_or_marker_p,
 
 DEFUN ("floatp", Ffloatp, Sfloatp, 1, 1, 0,
        doc: /* Return t if OBJECT is a floating point number.  */)
+/* CONST */
   (Lisp_Object object)
 {
   if (FLOATP (object))
@@ -2954,7 +2959,7 @@ DEFUN ("lognot", Flognot, Slognot, 1, 1, 0,
 DEFUN ("byteorder", Fbyteorder, Sbyteorder, 0, 0, 0,
        doc: /* Return the byteorder for the machine.
 Returns 66 (ASCII uppercase B) for big endian machines or 108 (ASCII
-lowercase l) for small endian machines.  */)
+lowercase l) for small endian machines.  */) /* CONST */
   (void)
 {
   unsigned i = 0x04030201;
diff --git a/src/decompress.c b/src/decompress.c
index b14f0a2..1dfd815 100644
--- a/src/decompress.c
+++ b/src/decompress.c
@@ -89,6 +89,7 @@ unwind_decompress (void *ddata)
 
 DEFUN ("zlib-available-p", Fzlib_available_p, Szlib_available_p, 0, 0, 0,
        doc: /* Return t if zlib decompression is available in this instance of Emacs.  */)
+/* CONST */
      (void)
 {
 #ifdef WINDOWSNT
diff --git a/src/emacs.c b/src/emacs.c
index d09c3c3..cef3632 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -1892,7 +1892,7 @@ or SIGHUP, and upon SIGINT in batch mode.
 
 The value of `kill-emacs-hook', if not void,
 is a list of functions (of no args),
-all of which are called before Emacs is actually killed.  */)
+all of which are called before Emacs is actually killed.  */) /* NORETURN */
   (Lisp_Object arg)
 {
   struct gcpro gcpro1;
diff --git a/src/eval.c b/src/eval.c
index 7e4b016..02964e3 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1162,7 +1162,7 @@ unwind_to_catch (struct handler *catch, Lisp_Object value)
 
 DEFUN ("throw", Fthrow, Sthrow, 2, 2, 0,
        doc: /* Throw to the catch for TAG and return VALUE from it.
-Both TAG and VALUE are evalled.  */)
+Both TAG and VALUE are evalled.  */) /* NORETURN */
   (register Lisp_Object tag, Lisp_Object value)
 {
   struct handler *c;
diff --git a/src/fns.c b/src/fns.c
index 7739663..94538d8 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -46,7 +46,7 @@ static void sort_vector_copy (Lisp_Object, ptrdiff_t,
 static bool internal_equal (Lisp_Object, Lisp_Object, int, bool, Lisp_Object);
 
 DEFUN ("identity", Fidentity, Sidentity, 1, 1, 0,
-       doc: /* Return the argument unchanged.  */)
+       doc: /* Return the argument unchanged.  */) /* CONST */
   (Lisp_Object arg)
 {
   return arg;
diff --git a/src/frame.c b/src/frame.c
index 3d2ffbf..69aa096 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -267,7 +267,7 @@ predicates which report frame's specific UI-related capabilities.  */)
 /* Placeholder used by temacs -nw before window.el is loaded.  */
 DEFUN ("frame-windows-min-size", Fframe_windows_min_size,
        Sframe_windows_min_size, 4, 4, 0,
-       doc: /* */)
+       doc: /* */) /* CONST */
      (Lisp_Object frame, Lisp_Object horizontal,
       Lisp_Object ignore, Lisp_Object pixelwise)
 {
diff --git a/src/gnutls.c b/src/gnutls.c
index 75fe614..b385bca 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -695,7 +695,7 @@ See also `gnutls-boot'.  */)
 DEFUN ("gnutls-errorp", Fgnutls_errorp, Sgnutls_errorp, 1, 1, 0,
        doc: /* Return t if ERROR indicates a GnuTLS problem.
 ERROR is an integer or a symbol with an integer `gnutls-code' property.
-usage: (gnutls-errorp ERROR)  */)
+usage: (gnutls-errorp ERROR)  */) /* CONST */
   (Lisp_Object err)
 {
   if (EQ (err, Qt)) return Qnil;
@@ -1604,6 +1604,7 @@ This function may also return `gnutls-e-again', or
 
 DEFUN ("gnutls-available-p", Fgnutls_available_p, Sgnutls_available_p, 0, 0, 0,
        doc: /* Return t if GnuTLS is available in this instance of Emacs.  */)
+/* CONST */
      (void)
 {
 #ifdef HAVE_GNUTLS
diff --git a/src/image.c b/src/image.c
index 5d08a89..9c09c55 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9288,7 +9288,8 @@ DEFUN ("imagep", Fimagep, Simagep, 1, 1, 0,
 }
 
 
-DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0, "")
+DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0,
+       doc: /* */)
   (Lisp_Object spec)
 {
   ptrdiff_t id = -1;
diff --git a/src/keyboard.c b/src/keyboard.c
index 5411aff..c04bbda 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1163,7 +1163,7 @@ top_level_1 (Lisp_Object ignore)
 
 DEFUN ("top-level", Ftop_level, Stop_level, 0, 0, "",
        doc: /* Exit all recursive editing levels.
-This also exits all active minibuffers.  */)
+This also exits all active minibuffers.  */) /* NORETURN */
   (void)
 {
 #ifdef HAVE_WINDOW_SYSTEM
@@ -1187,6 +1187,7 @@ user_error (const char *msg)
 /* _Noreturn will be added to prototype by make-docfile.  */
 DEFUN ("exit-recursive-edit", Fexit_recursive_edit, Sexit_recursive_edit, 0, 0, "",
        doc: /* Exit from the innermost recursive edit or minibuffer.  */)
+/* NORETURN */
   (void)
 {
   if (command_loop_level > 0 || minibuf_level > 0)
@@ -1198,6 +1199,7 @@ DEFUN ("exit-recursive-edit", Fexit_recursive_edit, Sexit_recursive_edit, 0, 0,
 /* _Noreturn will be added to prototype by make-docfile.  */
 DEFUN ("abort-recursive-edit", Fabort_recursive_edit, Sabort_recursive_edit, 0, 0, "",
        doc: /* Abort the command that requested this recursive edit or minibuffer input.  */)
+/* NORETURN */
   (void)
 {
   if (command_loop_level > 0 || minibuf_level > 0)
diff --git a/src/window.c b/src/window.c
index 1d2221f..af03300 100644
--- a/src/window.c
+++ b/src/window.c
@@ -3000,7 +3000,7 @@ resize_root_window (Lisp_Object window, Lisp_Object delta, Lisp_Object horizonta
 /* Placeholder used by temacs -nw before window.el is loaded.  */
 DEFUN ("window--sanitize-window-sizes", Fwindow__sanitize_window_sizes,
        Swindow__sanitize_window_sizes, 2, 2, 0,
-       doc: /* */)
+       doc: /* */) /* CONST */
      (Lisp_Object frame, Lisp_Object horizontal)
 {
   return Qnil;
diff --git a/src/xdisp.c b/src/xdisp.c
index 31702ed..7373501 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -12277,6 +12277,7 @@ DEFUN ("tool-bar-height", Ftool_bar_height, Stool_bar_height,
        doc: /* Return the number of lines occupied by the tool bar of FRAME.
 If FRAME is nil or omitted, use the selected frame.  Optional argument
 PIXELWISE non-nil means return the height of the tool bar in pixels.  */)
+/* CONST */
   (Lisp_Object frame, Lisp_Object pixelwise)
 {
   int height = 0;
diff --git a/src/xfaces.c b/src/xfaces.c
index 6ecd857..4077702 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3547,6 +3547,7 @@ A relative value is one that doesn't entirely override whatever is
 inherited from another face.  For most possible attributes,
 the only relative value that users see is `unspecified'.
 However, for :height, floating point values are also relative.  */)
+/* CONST */
   (Lisp_Object attribute, Lisp_Object value)
 {
   if (EQ (value, Qunspecified) || (EQ (value, QCignore_defface)))

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: Function attributes for make-docfile
  2015-01-12  4:09 Function attributes for make-docfile Dmitry Antipov
@ 2015-01-12  5:33 ` Stefan Monnier
  2015-01-12  5:49 ` Paul Eggert
  1 sibling, 0 replies; 15+ messages in thread
From: Stefan Monnier @ 2015-01-12  5:33 UTC (permalink / raw)
  To: Dmitry Antipov; +Cc: Paul Eggert, Emacs development discussions

> The following patch helps make-docfile to generate _Noreturn and
> ATTRIBUTE_CONST by looking at special comments found immediately after
> DEFUN declaration.  This code works only for regular-form docstrings
> (doc: /* xxx */), so Flookup_image is also fixed (the latter should be
> done anyway, IMO).  Any thoughts?

Nice, thanks,


        Stefan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Function attributes for make-docfile
  2015-01-12  4:09 Function attributes for make-docfile Dmitry Antipov
  2015-01-12  5:33 ` Stefan Monnier
@ 2015-01-12  5:49 ` Paul Eggert
  2015-01-12 12:28   ` #2 [Was: Re: Function attributes for make-docfile] Dmitry Antipov
  1 sibling, 1 reply; 15+ messages in thread
From: Paul Eggert @ 2015-01-12  5:49 UTC (permalink / raw)
  To: Dmitry Antipov, Emacs development discussions

Dmitry Antipov wrote:
>   DEFUN ("eq", Feq, Seq, 2, 2, 0,
>          doc: /* Return t if the two args are the same Lisp object.  */)
> +/* CONST */

Good idea, but I worry that the comments are fragile.  How about the following 
idea for improving the proposed syntax?  Move the directives to be inside the 
DEFUN, making it clearer that they're part of the function definition mechanism. 
  Something like this, say:

   DEFUN ("eq", Feq, Seq, 2, 2, 0,
          doc: /* Return t if the two args are the same Lisp object.  */
          attribute: const)

and similarly for 'noreturn' instead of (or in addition to) 'const'.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* #2 [Was: Re: Function attributes for make-docfile]
  2015-01-12  5:49 ` Paul Eggert
@ 2015-01-12 12:28   ` Dmitry Antipov
  2015-01-13  7:29     ` Paul Eggert
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Antipov @ 2015-01-12 12:28 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Stefan Monnier, Emacs development discussions

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

On 01/12/2015 08:49 AM, Paul Eggert wrote:

> Good idea, but I worry that the comments are fragile.  How about the following
> idea for improving the proposed syntax?  Move the directives to be inside the
> DEFUN, making it clearer that they're part
> of the function definition mechanism.  Something like this, say:
>
>    DEFUN ("eq", Feq, Seq, 2, 2, 0,
>           doc: /* Return t if the two args are the same Lisp object.  */
>           attribute: const)
>
> and similarly for 'noreturn' instead of (or in addition to) 'const'.

Ah yes, this looks better, and not too hard to (re)implement.  The only
minor issue is that 'const' is unconditionally highlighted as a C keyword
but 'noreturn' isn't.

OK to install?

Dmitry


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: function_attributes_2.patch --]
[-- Type: text/x-diff; name="function_attributes_2.patch", Size: 18837 bytes --]

diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index bc5420e..79d421a 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -562,6 +562,7 @@ struct global
 {
   enum global_type type;
   char *name;
+  int flags;
   union
   {
     int value;
@@ -569,13 +570,16 @@ struct global
   } v;
 };
 
+/* Bit values for FLAGS field from the above.  Applied for DEFUNs only.  */
+enum { DEFUN_noreturn = 1, DEFUN_const = 2 };
+
 /* All the variable names we saw while scanning C sources in `-g'
    mode.  */
 int num_globals;
 int num_globals_allocated;
 struct global *globals;
 
-static void
+static struct global *
 add_global (enum global_type type, char *name, int value, char const *svalue)
 {
   /* Ignore the one non-symbol that can occur.  */
@@ -601,7 +605,10 @@ add_global (enum global_type type, char *name, int value, char const *svalue)
 	globals[num_globals - 1].v.svalue = svalue;
       else
 	globals[num_globals - 1].v.value = value;
+      globals[num_globals - 1].flags = 0;
+      return globals + num_globals - 1;
     }
+  return NULL;
 }
 
 static int
@@ -708,13 +715,7 @@ write_globals (void)
 		globals[i].name, globals[i].name, globals[i].name);
       else
 	{
-	  /* It would be nice to have a cleaner way to deal with these
-	     special hacks.  */
-	  if (strcmp (globals[i].name, "Fthrow") == 0
-	      || strcmp (globals[i].name, "Ftop_level") == 0
-	      || strcmp (globals[i].name, "Fkill_emacs") == 0
-	      || strcmp (globals[i].name, "Fexit_recursive_edit") == 0
-	      || strcmp (globals[i].name, "Fabort_recursive_edit") == 0)
+	  if (globals[i].flags & DEFUN_noreturn)
 	    fputs ("_Noreturn ", stdout);
 
 	  printf ("EXFUN (%s, ", globals[i].name);
@@ -726,36 +727,7 @@ write_globals (void)
 	    printf ("%d", globals[i].v.value);
 	  putchar (')');
 
-	  /* It would be nice to have a cleaner way to deal with these
-	     special hacks, too.  */
-	  if (strcmp (globals[i].name, "Fatom") == 0
-	      || strcmp (globals[i].name, "Fbyteorder") == 0
-	      || strcmp (globals[i].name, "Fcharacterp") == 0
-	      || strcmp (globals[i].name, "Fchar_or_string_p") == 0
-	      || strcmp (globals[i].name, "Fconsp") == 0
-	      || strcmp (globals[i].name, "Feq") == 0
-	      || strcmp (globals[i].name, "Fface_attribute_relative_p") == 0
-	      || strcmp (globals[i].name, "Fframe_windows_min_size") == 0
-	      || strcmp (globals[i].name, "Fgnutls_errorp") == 0
-	      || strcmp (globals[i].name, "Fidentity") == 0
-	      || strcmp (globals[i].name, "Fintegerp") == 0
-	      || strcmp (globals[i].name, "Finteractive") == 0
-	      || strcmp (globals[i].name, "Ffloatp") == 0
-	      || strcmp (globals[i].name, "Flistp") == 0
-	      || strcmp (globals[i].name, "Fmax_char") == 0
-	      || strcmp (globals[i].name, "Fnatnump") == 0
-	      || strcmp (globals[i].name, "Fnlistp") == 0
-	      || strcmp (globals[i].name, "Fnull") == 0
-	      || strcmp (globals[i].name, "Fnumberp") == 0
-	      || strcmp (globals[i].name, "Fstringp") == 0
-	      || strcmp (globals[i].name, "Fsymbolp") == 0
-	      || strcmp (globals[i].name, "Ftool_bar_height") == 0
-	      || strcmp (globals[i].name, "Fwindow__sanitize_window_sizes") == 0
-#ifndef WINDOWSNT
-	      || strcmp (globals[i].name, "Fgnutls_available_p") == 0
-	      || strcmp (globals[i].name, "Fzlib_available_p") == 0
-#endif
-	      || 0)
+	  if (globals[i].flags & DEFUN_const)
 	    fputs (" ATTRIBUTE_CONST", stdout);
 
 	  puts (";");
@@ -817,6 +789,23 @@ scan_c_file (char *filename, const char *mode)
   return scan_c_stream (infile);
 }
 
+/* Return 1 if next input from INFILE is equal to P, -1 if EOF,
+   0 if input doesn't match.  */
+
+static int
+stream_match (FILE *infile, const char *p)
+{
+  for (; *p; p++)
+    {
+      int c = getc (infile);
+      if (c == EOF)
+       return -1;
+      if (c != *p)
+       return 0;
+    }
+  return 1;
+}
+
 static int
 scan_c_stream (FILE *infile)
 {
@@ -1033,7 +1022,63 @@ scan_c_stream (FILE *infile)
 
       if (generate_globals)
 	{
-	  add_global (FUNCTION, name, maxargs, 0);
+	  struct global *g = add_global (FUNCTION, name, maxargs, 0);
+
+	  /* The following code tries to recognize function attributes
+	     specified after the docstring, e.g.:
+
+	     DEFUN ("foo", Ffoo, Sfoo, X, Y, Z,
+		   doc: /\* doc *\/
+		   attributes: attribute1 attribute2 ...)
+	       (Lisp_Object arg...)
+
+	     Now only 'noreturn' and 'const' attributes are used.  */
+
+	  /* Advance to the end of docstring.  */
+	  c = getc (infile);
+	  if (c == EOF)
+	    goto eof;
+	  int d = getc (infile);
+	  if (d == EOF)
+	    goto eof;
+	  while (1)
+	    {
+	      if (c == '*' && d == '/')
+		break;
+	      c = d, d = getc (infile);
+	      if (d == EOF)
+		goto eof;
+	    }
+	  /* Skip spaces, if any.  */
+	  do
+	    {
+	      c = getc (infile);
+	      if (c == EOF)
+		goto eof;
+	    }
+	  while (c == ' ' || c == '\n' || c == '\r' || c == '\t');
+	  /* Check for 'attributes:' token.  */
+	  if (c == 'a' && stream_match (infile, "ttributes:"))
+	    {
+	      char *p = input_buffer;
+	      /* Collect attributes up to ')'.  */
+	      while (1)
+		{
+		  c = getc (infile);
+		  if (c == EOF)
+		    goto eof;
+		  if (c == ')')
+		    break;
+		  if (p - input_buffer > sizeof (input_buffer))
+		    abort ();
+		  *p++ = c;
+		}
+	      *p = 0;
+	      if (strstr (input_buffer, "noreturn"))
+		g->flags |= DEFUN_noreturn;
+	      if (strstr (input_buffer, "const"))
+		g->flags |= DEFUN_const;
+	    }
 	  continue;
 	}
 
diff --git a/src/callint.c b/src/callint.c
index 2595503..dd238b9 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -101,7 +101,8 @@ If the string begins with `^' and `shift-select-mode' is non-nil,
  Emacs first calls the function `handle-shift-selection'.
 You may use `@', `*', and `^' together.  They are processed in the
  order that they appear, before reading any arguments.
-usage: (interactive &optional ARGS)  */)
+usage: (interactive &optional ARGS)  */
+       attributes: const)
   (Lisp_Object args)
 {
   return Qnil;
diff --git a/src/character.c b/src/character.c
index 4a5c7ec..39d32c9 100644
--- a/src/character.c
+++ b/src/character.c
@@ -232,14 +232,16 @@ DEFUN ("characterp", Fcharacterp, Scharacterp, 1, 2, 0,
 In Emacs Lisp, characters are represented by character codes, which
 are non-negative integers.  The function `max-char' returns the
 maximum character code.
-usage: (characterp OBJECT)  */)
+usage: (characterp OBJECT)  */
+       attributes: const)
   (Lisp_Object object, Lisp_Object ignore)
 {
   return (CHARACTERP (object) ? Qt : Qnil);
 }
 
 DEFUN ("max-char", Fmax_char, Smax_char, 0, 0, 0,
-       doc: /* Return the character of the maximum code.  */)
+       doc: /* Return the character of the maximum code.  */
+       attributes: const)
   (void)
 {
   return make_number (MAX_CHAR);
diff --git a/src/data.c b/src/data.c
index 820c3ce..0389eb4 100644
--- a/src/data.c
+++ b/src/data.c
@@ -176,7 +176,8 @@ args_out_of_range_3 (Lisp_Object a1, Lisp_Object a2, Lisp_Object a3)
 /* Data type predicates.  */
 
 DEFUN ("eq", Feq, Seq, 2, 2, 0,
-       doc: /* Return t if the two args are the same Lisp object.  */)
+       doc: /* Return t if the two args are the same Lisp object.  */
+       attributes: const)
   (Lisp_Object obj1, Lisp_Object obj2)
 {
   if (EQ (obj1, obj2))
@@ -185,7 +186,8 @@ DEFUN ("eq", Feq, Seq, 2, 2, 0,
 }
 
 DEFUN ("null", Fnull, Snull, 1, 1, 0,
-       doc: /* Return t if OBJECT is nil.  */)
+       doc: /* Return t if OBJECT is nil.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (NILP (object))
@@ -263,7 +265,8 @@ for example, (type-of 1) returns `integer'.  */)
 }
 
 DEFUN ("consp", Fconsp, Sconsp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a cons cell.  */)
+       doc: /* Return t if OBJECT is a cons cell.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (CONSP (object))
@@ -272,7 +275,8 @@ DEFUN ("consp", Fconsp, Sconsp, 1, 1, 0,
 }
 
 DEFUN ("atom", Fatom, Satom, 1, 1, 0,
-       doc: /* Return t if OBJECT is not a cons cell.  This includes nil.  */)
+       doc: /* Return t if OBJECT is not a cons cell.  This includes nil.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (CONSP (object))
@@ -282,7 +286,8 @@ DEFUN ("atom", Fatom, Satom, 1, 1, 0,
 
 DEFUN ("listp", Flistp, Slistp, 1, 1, 0,
        doc: /* Return t if OBJECT is a list, that is, a cons cell or nil.
-Otherwise, return nil.  */)
+Otherwise, return nil.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (CONSP (object) || NILP (object))
@@ -291,7 +296,8 @@ Otherwise, return nil.  */)
 }
 
 DEFUN ("nlistp", Fnlistp, Snlistp, 1, 1, 0,
-       doc: /* Return t if OBJECT is not a list.  Lists include nil.  */)
+       doc: /* Return t if OBJECT is not a list.  Lists include nil.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (CONSP (object) || NILP (object))
@@ -300,7 +306,8 @@ DEFUN ("nlistp", Fnlistp, Snlistp, 1, 1, 0,
 }
 \f
 DEFUN ("symbolp", Fsymbolp, Ssymbolp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a symbol.  */)
+       doc: /* Return t if OBJECT is a symbol.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (SYMBOLP (object))
@@ -333,7 +340,8 @@ DEFUN ("vectorp", Fvectorp, Svectorp, 1, 1, 0,
 }
 
 DEFUN ("stringp", Fstringp, Sstringp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a string.  */)
+       doc: /* Return t if OBJECT is a string.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (STRINGP (object))
@@ -436,7 +444,8 @@ DEFUN ("byte-code-function-p", Fbyte_code_function_p, Sbyte_code_function_p,
 }
 
 DEFUN ("char-or-string-p", Fchar_or_string_p, Schar_or_string_p, 1, 1, 0,
-       doc: /* Return t if OBJECT is a character or a string.  */)
+       doc: /* Return t if OBJECT is a character or a string.  */
+       attributes: const)
   (register Lisp_Object object)
 {
   if (CHARACTERP (object) || STRINGP (object))
@@ -445,7 +454,8 @@ DEFUN ("char-or-string-p", Fchar_or_string_p, Schar_or_string_p, 1, 1, 0,
 }
 \f
 DEFUN ("integerp", Fintegerp, Sintegerp, 1, 1, 0,
-       doc: /* Return t if OBJECT is an integer.  */)
+       doc: /* Return t if OBJECT is an integer.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (INTEGERP (object))
@@ -463,7 +473,8 @@ DEFUN ("integer-or-marker-p", Finteger_or_marker_p, Sinteger_or_marker_p, 1, 1,
 }
 
 DEFUN ("natnump", Fnatnump, Snatnump, 1, 1, 0,
-       doc: /* Return t if OBJECT is a nonnegative integer.  */)
+       doc: /* Return t if OBJECT is a nonnegative integer.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (NATNUMP (object))
@@ -472,7 +483,8 @@ DEFUN ("natnump", Fnatnump, Snatnump, 1, 1, 0,
 }
 
 DEFUN ("numberp", Fnumberp, Snumberp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a number (floating point or integer).  */)
+       doc: /* Return t if OBJECT is a number (floating point or integer).  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (NUMBERP (object))
@@ -492,7 +504,8 @@ DEFUN ("number-or-marker-p", Fnumber_or_marker_p,
 }
 
 DEFUN ("floatp", Ffloatp, Sfloatp, 1, 1, 0,
-       doc: /* Return t if OBJECT is a floating point number.  */)
+       doc: /* Return t if OBJECT is a floating point number.  */
+       attributes: const)
   (Lisp_Object object)
 {
   if (FLOATP (object))
@@ -2954,7 +2967,8 @@ DEFUN ("lognot", Flognot, Slognot, 1, 1, 0,
 DEFUN ("byteorder", Fbyteorder, Sbyteorder, 0, 0, 0,
        doc: /* Return the byteorder for the machine.
 Returns 66 (ASCII uppercase B) for big endian machines or 108 (ASCII
-lowercase l) for small endian machines.  */)
+lowercase l) for small endian machines.  */
+       attributes: const)
   (void)
 {
   unsigned i = 0x04030201;
diff --git a/src/decompress.c b/src/decompress.c
index b14f0a2..b78dace 100644
--- a/src/decompress.c
+++ b/src/decompress.c
@@ -88,7 +88,8 @@ unwind_decompress (void *ddata)
 }
 
 DEFUN ("zlib-available-p", Fzlib_available_p, Szlib_available_p, 0, 0, 0,
-       doc: /* Return t if zlib decompression is available in this instance of Emacs.  */)
+       doc: /* Return t if zlib decompression is available in this instance of Emacs.  */
+       attributes: const)
      (void)
 {
 #ifdef WINDOWSNT
diff --git a/src/emacs.c b/src/emacs.c
index d09c3c3..ca1a8b2 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -1892,7 +1892,8 @@ or SIGHUP, and upon SIGINT in batch mode.
 
 The value of `kill-emacs-hook', if not void,
 is a list of functions (of no args),
-all of which are called before Emacs is actually killed.  */)
+all of which are called before Emacs is actually killed.  */
+       attributes: noreturn)
   (Lisp_Object arg)
 {
   struct gcpro gcpro1;
diff --git a/src/eval.c b/src/eval.c
index 7e4b016..5cadb1b 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1162,7 +1162,8 @@ unwind_to_catch (struct handler *catch, Lisp_Object value)
 
 DEFUN ("throw", Fthrow, Sthrow, 2, 2, 0,
        doc: /* Throw to the catch for TAG and return VALUE from it.
-Both TAG and VALUE are evalled.  */)
+Both TAG and VALUE are evalled.  */
+       attributes: noreturn)
   (register Lisp_Object tag, Lisp_Object value)
 {
   struct handler *c;
diff --git a/src/fns.c b/src/fns.c
index 7739663..91cd513 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -46,7 +46,8 @@ static void sort_vector_copy (Lisp_Object, ptrdiff_t,
 static bool internal_equal (Lisp_Object, Lisp_Object, int, bool, Lisp_Object);
 
 DEFUN ("identity", Fidentity, Sidentity, 1, 1, 0,
-       doc: /* Return the argument unchanged.  */)
+       doc: /* Return the argument unchanged.  */
+       attributes: const)
   (Lisp_Object arg)
 {
   return arg;
diff --git a/src/frame.c b/src/frame.c
index 3d2ffbf..150c058 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -267,7 +267,8 @@ predicates which report frame's specific UI-related capabilities.  */)
 /* Placeholder used by temacs -nw before window.el is loaded.  */
 DEFUN ("frame-windows-min-size", Fframe_windows_min_size,
        Sframe_windows_min_size, 4, 4, 0,
-       doc: /* */)
+       doc: /* */
+       attributes: const)
      (Lisp_Object frame, Lisp_Object horizontal,
       Lisp_Object ignore, Lisp_Object pixelwise)
 {
diff --git a/src/gnutls.c b/src/gnutls.c
index 75fe614..5e6c635 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -695,7 +695,8 @@ See also `gnutls-boot'.  */)
 DEFUN ("gnutls-errorp", Fgnutls_errorp, Sgnutls_errorp, 1, 1, 0,
        doc: /* Return t if ERROR indicates a GnuTLS problem.
 ERROR is an integer or a symbol with an integer `gnutls-code' property.
-usage: (gnutls-errorp ERROR)  */)
+usage: (gnutls-errorp ERROR)  */
+       attributes: const)
   (Lisp_Object err)
 {
   if (EQ (err, Qt)) return Qnil;
@@ -1603,7 +1604,8 @@ This function may also return `gnutls-e-again', or
 #endif	/* HAVE_GNUTLS */
 
 DEFUN ("gnutls-available-p", Fgnutls_available_p, Sgnutls_available_p, 0, 0, 0,
-       doc: /* Return t if GnuTLS is available in this instance of Emacs.  */)
+       doc: /* Return t if GnuTLS is available in this instance of Emacs.  */
+       attributes: const)
      (void)
 {
 #ifdef HAVE_GNUTLS
diff --git a/src/keyboard.c b/src/keyboard.c
index 5411aff..c1207f8 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1163,7 +1163,8 @@ top_level_1 (Lisp_Object ignore)
 
 DEFUN ("top-level", Ftop_level, Stop_level, 0, 0, "",
        doc: /* Exit all recursive editing levels.
-This also exits all active minibuffers.  */)
+This also exits all active minibuffers.  */
+       attributes: noreturn)
   (void)
 {
 #ifdef HAVE_WINDOW_SYSTEM
@@ -1186,7 +1187,8 @@ user_error (const char *msg)
 
 /* _Noreturn will be added to prototype by make-docfile.  */
 DEFUN ("exit-recursive-edit", Fexit_recursive_edit, Sexit_recursive_edit, 0, 0, "",
-       doc: /* Exit from the innermost recursive edit or minibuffer.  */)
+       doc: /* Exit from the innermost recursive edit or minibuffer.  */
+       attributes: noreturn)
   (void)
 {
   if (command_loop_level > 0 || minibuf_level > 0)
@@ -1197,7 +1199,8 @@ DEFUN ("exit-recursive-edit", Fexit_recursive_edit, Sexit_recursive_edit, 0, 0,
 
 /* _Noreturn will be added to prototype by make-docfile.  */
 DEFUN ("abort-recursive-edit", Fabort_recursive_edit, Sabort_recursive_edit, 0, 0, "",
-       doc: /* Abort the command that requested this recursive edit or minibuffer input.  */)
+       doc: /* Abort the command that requested this recursive edit or minibuffer input.  */
+       attributes: noreturn)
   (void)
 {
   if (command_loop_level > 0 || minibuf_level > 0)
diff --git a/src/window.c b/src/window.c
index 1d2221f..e11299e 100644
--- a/src/window.c
+++ b/src/window.c
@@ -3000,7 +3000,8 @@ resize_root_window (Lisp_Object window, Lisp_Object delta, Lisp_Object horizonta
 /* Placeholder used by temacs -nw before window.el is loaded.  */
 DEFUN ("window--sanitize-window-sizes", Fwindow__sanitize_window_sizes,
        Swindow__sanitize_window_sizes, 2, 2, 0,
-       doc: /* */)
+       doc: /* */
+       attributes: const)
      (Lisp_Object frame, Lisp_Object horizontal)
 {
   return Qnil;
diff --git a/src/xdisp.c b/src/xdisp.c
index 31702ed..bb7ad19 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -12276,7 +12276,8 @@ DEFUN ("tool-bar-height", Ftool_bar_height, Stool_bar_height,
        0, 2, 0,
        doc: /* Return the number of lines occupied by the tool bar of FRAME.
 If FRAME is nil or omitted, use the selected frame.  Optional argument
-PIXELWISE non-nil means return the height of the tool bar in pixels.  */)
+PIXELWISE non-nil means return the height of the tool bar in pixels.  */
+       attributes: const)
   (Lisp_Object frame, Lisp_Object pixelwise)
 {
   int height = 0;
diff --git a/src/xfaces.c b/src/xfaces.c
index 6ecd857..85af770 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3546,7 +3546,8 @@ with the value VALUE is relative.
 A relative value is one that doesn't entirely override whatever is
 inherited from another face.  For most possible attributes,
 the only relative value that users see is `unspecified'.
-However, for :height, floating point values are also relative.  */)
+However, for :height, floating point values are also relative.  */
+       attributes: const)
   (Lisp_Object attribute, Lisp_Object value)
 {
   if (EQ (value, Qunspecified) || (EQ (value, QCignore_defface)))

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-12 12:28   ` #2 [Was: Re: Function attributes for make-docfile] Dmitry Antipov
@ 2015-01-13  7:29     ` Paul Eggert
  2015-01-13 10:21       ` Dmitry Antipov
  2015-01-13 19:45       ` Stefan Monnier
  0 siblings, 2 replies; 15+ messages in thread
From: Paul Eggert @ 2015-01-13  7:29 UTC (permalink / raw)
  To: Dmitry Antipov; +Cc: Emacs development discussions

Dmitry Antipov wrote:
> OK to install?

I guess so, since you already installed it.  :-)  It looks good, but there is a 
problem when I configure as follows on Fedora 21 x86-64:

./configure --enable-gcc-warnings --with-x-toolkit=no

The resulting build fails as follows:

   CC       fileio.o
In file included from fileio.c:45:0:
fileio.c: In function ‘Fnext_read_file_uses_dialog_p’:
fileio.c:5739:40: error: function might be candidate for attribute ‘const’ 
[-Werror=suggest-attribute=const]
  DEFUN ("next-read-file-uses-dialog-p", Fnext_read_file_uses_dialog_p,
                                         ^
lisp.h:2766:16: note: in definition of macro ‘DEFUN’
     Lisp_Object fnname
                 ^
cc1: all warnings being treated as errors


Fnext_read_file_uses_dialog_p should be a const function if and only if 
!(defined USE_MOTIF || defined HAVE_NTGUI || defined USE_GTK || defined 
HAVE_NS), but the DEFUN syntax doesn't let one specify a function to be 
conditionally const -- it's either always const or always non-const.  This 
problem existed before you installed this change, but I worry that the change 
will make it harder to fix.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-13  7:29     ` Paul Eggert
@ 2015-01-13 10:21       ` Dmitry Antipov
  2015-01-13 17:43         ` Paul Eggert
  2015-01-13 19:45       ` Stefan Monnier
  1 sibling, 1 reply; 15+ messages in thread
From: Dmitry Antipov @ 2015-01-13 10:21 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Emacs development discussions

On 01/13/2015 10:29 AM, Paul Eggert wrote:

> Fnext_read_file_uses_dialog_p should be a const function if and only if
> !(defined USE_MOTIF || defined HAVE_NTGUI || defined USE_GTK || defined HAVE_NS),
> but the DEFUN syntax doesn't let one specify a
> function to be conditionally const -- it's either always const or always non-const.
> This problem existed before you installed this change, but I worry that the change
> will make it harder to fix.

Hopefully 0064e36f4fc76b0e8d2fc8d3e6f63da6e579a414 should be a workaround for that.

Dmitry




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-13 10:21       ` Dmitry Antipov
@ 2015-01-13 17:43         ` Paul Eggert
  0 siblings, 0 replies; 15+ messages in thread
From: Paul Eggert @ 2015-01-13 17:43 UTC (permalink / raw)
  To: Dmitry Antipov; +Cc: Emacs development discussions

On 01/13/2015 02:21 AM, Dmitry Antipov wrote:
> Hopefully 0064e36f4fc76b0e8d2fc8d3e6f63da6e579a414 should be a 
> workaround for that. 

Doesn't that change cause the source code to lie to the C compiler? 
Fnext_read_file_uses_dialog_p isn't really a const function, since it 
can return both true and false in the same execution.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-13  7:29     ` Paul Eggert
  2015-01-13 10:21       ` Dmitry Antipov
@ 2015-01-13 19:45       ` Stefan Monnier
  2015-01-13 23:26         ` Paul Eggert
  1 sibling, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2015-01-13 19:45 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Dmitry Antipov, Emacs development discussions

> Fnext_read_file_uses_dialog_p should be a const function if and only
> if !(defined USE_MOTIF || defined HAVE_NTGUI || defined USE_GTK || defined
> HAVE_NS),

IOW it is *not* a const function.


        Stefan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-13 19:45       ` Stefan Monnier
@ 2015-01-13 23:26         ` Paul Eggert
  2015-01-14 17:49           ` Stefan Monnier
  0 siblings, 1 reply; 15+ messages in thread
From: Paul Eggert @ 2015-01-13 23:26 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Dmitry Antipov, Emacs development discussions

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

On 01/13/2015 11:45 AM, Stefan Monnier wrote:
> IOW it is*not*  a const function.

To fix this I installed the attached patch as master commit 
785adfcc8dee02ac544f80e4f7f8d3d5b2965981 
<http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=785adfcc8dee02ac544f80e4f7f8d3d5b2965981>. 
It's not pretty, but at least we're no longer lying to the compiler.

[-- Attachment #2: 0001-Don-t-say-Fnext_read_file_uses_dialog_p-is-const.patch --]
[-- Type: text/x-patch, Size: 4621 bytes --]

From c020d2f7eee565616dff21e9d22b3200a4d5debf Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 13 Jan 2015 15:22:19 -0800
Subject: [PATCH] Don't say Fnext_read_file_uses_dialog_p is const

It's const only if a windowing system is not used; don't say it's
const otherwise.  See:
http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00310.html
* lib-src/make-docfile.c (write_globals):
Add a special hack for Fnext_read_file_uses_dialog_p.
* src/fileio.c (next_read_file_uses_dialog_p): Remove.
Move guts back to ...
(Fnext_read_file_uses_dialog_p): ... here.
Don't declare as const, as make-docfile.c now has a special case
for this function.  This is an ugly hack, but it's better than
lying to the compiler.
---
 lib-src/ChangeLog      |  6 ++++++
 lib-src/make-docfile.c | 12 ++++++++++++
 src/ChangeLog          | 11 +++++++++++
 src/fileio.c           | 30 ++++++++++--------------------
 4 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog
index 969aac8..e9205fd 100644
--- a/lib-src/ChangeLog
+++ b/lib-src/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-13  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Don't say Fnext_read_file_uses_dialog_p is const
+	* make-docfile.c (write_globals):
+	Add a special hack for Fnext_read_file_uses_dialog_p.
+
 2015-01-13  Dmitry Antipov  <dmantipov@yandex.ru>
 
 	Support DEFUN attributes.
diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index 79d421a..741fa4b 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -729,6 +729,18 @@ write_globals (void)
 
 	  if (globals[i].flags & DEFUN_const)
 	    fputs (" ATTRIBUTE_CONST", stdout);
+	  else if (strcmp (globals[i].name, "Fnext_read_file_uses_dialog_p")
+		   == 0)
+	    {
+	      /* It would be nice to have a cleaner way to deal with this
+		 special hack.  */
+	      fputs (("\n"
+		      "#if ! (defined USE_GTK || defined USE_MOTIF \\\n"
+		      "       || defined HAVE_NS || defined HAVE_NTGUI)\n"
+		      "\tATTRIBUTE_CONST\n"
+		      "#endif\n"),
+		     stdout);
+	    }
 
 	  puts (";");
 	}
diff --git a/src/ChangeLog b/src/ChangeLog
index 7ec6980..8d05ec1 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,5 +1,16 @@
 2015-01-13  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Don't say Fnext_read_file_uses_dialog_p is const
+	It's const only if a windowing system is not used; don't say it's
+	const otherwise.  See:
+	http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00310.html
+	* fileio.c (next_read_file_uses_dialog_p): Remove.
+	Move guts back to ...
+	(Fnext_read_file_uses_dialog_p): ... here.
+	Don't declare as const, as make-docfile.c now has a special case
+	for this function.  This is an ugly hack, but it's better than
+	lying to the compiler.
+
 	Remove now-unnecessary forward XTYPE decl
 	* lisp.h (XTYPE): Remove forward declaration.  The recent merge
 	from emacs-24 fixed the problem in a better way, by moving XPNTR's
diff --git a/src/fileio.c b/src/fileio.c
index 45a31c0..6c443c9 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5734,34 +5734,24 @@ then any auto-save counts as "recent".  */)
   return (SAVE_MODIFF < BUF_AUTOSAVE_MODIFF (current_buffer) ? Qt : Qnil);
 }
 
-/* We want Fnext_read_file_uses_dialog_p to have ATTRIBUTE_CONST
-   regardless of #ifdefs, so there is a trivial workaround.  See
-   http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00289.html.  */
-
-static bool
-next_read_file_uses_dialog_p (void)
-{
-#if defined (USE_MOTIF) || defined (HAVE_NTGUI) || defined (USE_GTK) \
-  || defined (HAVE_NS)
-  return ((NILP (last_nonmenu_event) || CONSP (last_nonmenu_event))
-	  && use_dialog_box
-	  && use_file_dialog
-	  && window_system_available (SELECTED_FRAME ()));
-#endif
-  return false;
-}
-
 /* Reading and completing file names.  */
 
 DEFUN ("next-read-file-uses-dialog-p", Fnext_read_file_uses_dialog_p,
        Snext_read_file_uses_dialog_p, 0, 0, 0,
        doc: /* Return t if a call to `read-file-name' will use a dialog.
 The return value is only relevant for a call to `read-file-name' that happens
-before any other event (mouse or keypress) is handled.  */
-       attributes: const)
+before any other event (mouse or keypress) is handled.  */)
   (void)
 {
-  return next_read_file_uses_dialog_p () ? Qt : Qnil;
+#if (defined USE_GTK || defined USE_MOTIF \
+     || defined HAVE_NS || defined HAVE_NTGUI)
+  if ((NILP (last_nonmenu_event) || CONSP (last_nonmenu_event))
+      && use_dialog_box
+      && use_file_dialog
+      && window_system_available (SELECTED_FRAME ()))
+    return Qt;
+#endif
+  return Qnil;
 }
 
 void
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-13 23:26         ` Paul Eggert
@ 2015-01-14 17:49           ` Stefan Monnier
  2015-01-14 19:44             ` Paul Eggert
  0 siblings, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2015-01-14 17:49 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Dmitry Antipov, Emacs development discussions

> +	  else if (strcmp (globals[i].name, "Fnext_read_file_uses_dialog_p")
> +		   == 0)
> +	    {
> +	      /* It would be nice to have a cleaner way to deal with this
> +		 special hack.  */
> +	      fputs (("\n"
> +		      "#if ! (defined USE_GTK || defined USE_MOTIF \\\n"
> +		      "       || defined HAVE_NS || defined HAVE_NTGUI)\n"
> +		      "\tATTRIBUTE_CONST\n"
> +		      "#endif\n"),
> +		     stdout);
> +	    }

This is hideous.  Why do we need that?

> -before any other event (mouse or keypress) is handled.  */
> -       attributes: const)
> +before any other event (mouse or keypress) is handled.  */)

God,


        Stefan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-14 17:49           ` Stefan Monnier
@ 2015-01-14 19:44             ` Paul Eggert
  2015-01-15  3:27               ` Dmitry Antipov
  2015-01-15  5:33               ` Stefan Monnier
  0 siblings, 2 replies; 15+ messages in thread
From: Paul Eggert @ 2015-01-14 19:44 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Dmitry Antipov, Emacs development discussions

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

On 01/14/2015 09:49 AM, Stefan Monnier wrote:
> This is hideous.  Why do we need that?

Assuming --enable-gcc-warnings, it's a combination of things. First, gcc 
-Wsuggest-attribute=const complains if it can deduce that an extern 
function is const but the function wasn't declared const. Second, this 
particular function is const on some platforms and not on others, so we 
can't simply declare it to be const everywhere. Third, the DEFUN syntax 
doesn't give us a way to declare a function const only on some 
platforms.  Fourth, gcc -Wredundant-decls won't let us supply another 
extern declaration by hand, with the const attribute on platforms that 
need const, because GCC complains if there are duplicate extern declations.

There are several possibilities for how to fix this in a less-ugly way, 
including:

1.  Omit -Wsuggest-attribute=const.

2.  Omit -Wredundant-decls.

3.  Make make-docfile.c even smarter and/or trickier.

The attached patch would do (2), as I expect this is simplest. Would you 
prefer this?

[-- Attachment #2: 0001-Give-up-on-Wredundant-decls.patch --]
[-- Type: text/x-patch, Size: 4732 bytes --]

From ed8c06dcd1442d2543a5272e741e1ec97f9f9090 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 14 Jan 2015 11:42:43 -0800
Subject: [PATCH] Give up on -Wredundant-decls

This removes the need for the ugly make-docfile.c hack involving
Fnext_read_file_uses_dialog_p.
* configure.ac (WARN_CFLAGS): Omit -Wredundant-decls.
* lib-src/make-docfile.c (write_globals): Undo previous change.
* src/fileio.c (READ_FILE_NAME_MAY_USE_DIALOG): New constant.
(Fnext_read_file_uses_dialog_p): Use it instead of an #ifdef.
Use a redundant decl to declare this function to be const, on
platforms where it's const.
---
 ChangeLog              |  5 +++++
 configure.ac           |  1 -
 lib-src/ChangeLog      |  7 +++++++
 lib-src/make-docfile.c | 12 ------------
 src/ChangeLog          |  8 ++++++++
 src/fileio.c           | 14 ++++++++++----
 6 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cca9100..a28b68e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-01-14  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Give up on -Wredundant-decls
+	* configure.ac (WARN_CFLAGS): Omit -Wredundant-decls.
+
 2015-01-11  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Default to 'configure --enable-silent-rules'
diff --git a/configure.ac b/configure.ac
index 4cad214..3600e00 100644
--- a/configure.ac
+++ b/configure.ac
@@ -912,7 +912,6 @@ else
   for w in $ws; do
     gl_WARN_ADD([$w])
   done
-  gl_WARN_ADD([-Wredundant-decls])     # Prefer this, as we don't use Bison.
   gl_WARN_ADD([-Wno-missing-field-initializers]) # We need this one
   gl_WARN_ADD([-Wno-sign-compare])     # Too many warnings for now
   gl_WARN_ADD([-Wno-type-limits])      # Too many warnings for now
diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog
index e9205fd..e4408c0 100644
--- a/lib-src/ChangeLog
+++ b/lib-src/ChangeLog
@@ -1,3 +1,10 @@
+2015-01-14  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Give up on -Wredundant-decls
+	This removes the need for the ugly make-docfile.c hack involving
+	Fnext_read_file_uses_dialog_p.
+	* make-docfile.c (write_globals): Undo previous change.
+
 2015-01-13  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Don't say Fnext_read_file_uses_dialog_p is const
diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index 741fa4b..79d421a 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -729,18 +729,6 @@ write_globals (void)
 
 	  if (globals[i].flags & DEFUN_const)
 	    fputs (" ATTRIBUTE_CONST", stdout);
-	  else if (strcmp (globals[i].name, "Fnext_read_file_uses_dialog_p")
-		   == 0)
-	    {
-	      /* It would be nice to have a cleaner way to deal with this
-		 special hack.  */
-	      fputs (("\n"
-		      "#if ! (defined USE_GTK || defined USE_MOTIF \\\n"
-		      "       || defined HAVE_NS || defined HAVE_NTGUI)\n"
-		      "\tATTRIBUTE_CONST\n"
-		      "#endif\n"),
-		     stdout);
-	    }
 
 	  puts (";");
 	}
diff --git a/src/ChangeLog b/src/ChangeLog
index b2588f1..b4929d0 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,11 @@
+2015-01-14  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Give up on -Wredundant-decls
+	* fileio.c (READ_FILE_NAME_MAY_USE_DIALOG): New constant.
+	(Fnext_read_file_uses_dialog_p): Use it instead of an #ifdef.
+	Use a redundant decl to declare this function to be const, on
+	platforms where it's const.
+
 2015-01-14  Eli Zaretskii  <eliz@gnu.org>
 
 	* w32fns.c (w32_wnd_proc): Ignore MENUITEMINFO's dwItemData data
diff --git a/src/fileio.c b/src/fileio.c
index 6c443c9..3237d74 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5736,6 +5736,14 @@ then any auto-save counts as "recent".  */)
 
 /* Reading and completing file names.  */
 
+#if (defined USE_GTK || defined USE_MOTIF \
+     || defined HAVE_NS || defined HAVE_NTGUI)
+enum { READ_FILE_NAME_MAY_USE_DIALOG = true };
+#else
+enum { READ_FILE_NAME_MAY_USE_DIALOG = false };
+EXFUN (Fnext_read_file_uses_dialog_p, 0) ATTRIBUTE_CONST;
+#endif
+
 DEFUN ("next-read-file-uses-dialog-p", Fnext_read_file_uses_dialog_p,
        Snext_read_file_uses_dialog_p, 0, 0, 0,
        doc: /* Return t if a call to `read-file-name' will use a dialog.
@@ -5743,14 +5751,12 @@ The return value is only relevant for a call to `read-file-name' that happens
 before any other event (mouse or keypress) is handled.  */)
   (void)
 {
-#if (defined USE_GTK || defined USE_MOTIF \
-     || defined HAVE_NS || defined HAVE_NTGUI)
-  if ((NILP (last_nonmenu_event) || CONSP (last_nonmenu_event))
+  if (READ_FILE_NAME_MAY_USE_DIALOG
+      && (NILP (last_nonmenu_event) || CONSP (last_nonmenu_event))
       && use_dialog_box
       && use_file_dialog
       && window_system_available (SELECTED_FRAME ()))
     return Qt;
-#endif
   return Qnil;
 }
 
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-14 19:44             ` Paul Eggert
@ 2015-01-15  3:27               ` Dmitry Antipov
  2015-01-15  3:54                 ` Dmitry Antipov
  2015-01-15  5:33               ` Stefan Monnier
  1 sibling, 1 reply; 15+ messages in thread
From: Dmitry Antipov @ 2015-01-15  3:27 UTC (permalink / raw)
  To: Paul Eggert, Stefan Monnier; +Cc: Emacs development discussions

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

On 01/14/2015 10:44 PM, Paul Eggert wrote:

> There are several possibilities for how to fix this in a less-ugly way, including:
>
> 1.  Omit -Wsuggest-attribute=const.
>
> 2.  Omit -Wredundant-decls.
>
> 3.  Make make-docfile.c even smarter and/or trickier.

2) was added to cleanup global namespace, which is typically polluted in mature
projects, and I think this is still useful.

Why not use #pragma in the way similar to bytecode.c?  IMO local ugliness is
always better than the global one :-).

Dmitry


[-- Attachment #2: pragma_disable_warning.patch --]
[-- Type: text/x-diff, Size: 1472 bytes --]

diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index 741fa4b..79d421a 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -729,18 +729,6 @@ write_globals (void)
 
 	  if (globals[i].flags & DEFUN_const)
 	    fputs (" ATTRIBUTE_CONST", stdout);
-	  else if (strcmp (globals[i].name, "Fnext_read_file_uses_dialog_p")
-		   == 0)
-	    {
-	      /* It would be nice to have a cleaner way to deal with this
-		 special hack.  */
-	      fputs (("\n"
-		      "#if ! (defined USE_GTK || defined USE_MOTIF \\\n"
-		      "       || defined HAVE_NS || defined HAVE_NTGUI)\n"
-		      "\tATTRIBUTE_CONST\n"
-		      "#endif\n"),
-		     stdout);
-	    }
 
 	  puts (";");
 	}
diff --git a/src/fileio.c b/src/fileio.c
index 6c443c9..31b117b 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5736,6 +5736,11 @@ then any auto-save counts as "recent".  */)
 
 /* Reading and completing file names.  */
 
+#if 4 < __GNUC__ + (6 <= __GNUC_MINOR__)
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wsuggest-attribute=const"
+#endif
+
 DEFUN ("next-read-file-uses-dialog-p", Fnext_read_file_uses_dialog_p,
        Snext_read_file_uses_dialog_p, 0, 0, 0,
        doc: /* Return t if a call to `read-file-name' will use a dialog.
@@ -5754,6 +5759,10 @@ before any other event (mouse or keypress) is handled.  */)
   return Qnil;
 }
 
+#if 4 < __GNUC__ + (6 <= __GNUC_MINOR__)
+# pragma GCC diagnostic pop
+#endif
+
 void
 init_fileio (void)
 {

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-15  3:27               ` Dmitry Antipov
@ 2015-01-15  3:54                 ` Dmitry Antipov
  0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Antipov @ 2015-01-15  3:54 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Stefan Monnier, Emacs development discussions

BTW, '--enable-gcc-warnings --with-x-toolkit=no' build should have the
Fnext_read_file_uses_dialog_p-like issue with Fx_uses_old_gtk_dialog.
But I don't see it, and don't understand why :-(.

Dmitry





^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-14 19:44             ` Paul Eggert
  2015-01-15  3:27               ` Dmitry Antipov
@ 2015-01-15  5:33               ` Stefan Monnier
  2015-01-16  4:50                 ` Paul Eggert
  1 sibling, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2015-01-15  5:33 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Dmitry Antipov, Emacs development discussions

> Assuming --enable-gcc-warnings, it's a combination of things. First,
> gcc -Wsuggest-attribute=const complains if it can deduce that an extern
> function is const but the function wasn't declared const.

Easy, then: either you live with this warning, or you don't
enable -Wsuggest-attribute=const.

Just silencing such a warning is no justification for this kind of
hideous code.

Using -Wsuggest-attribute=const every once in a while to see what could
be improved is probably a good idea, but we shouldn't blindly follow its
suggestions.


        Stefan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: #2 [Was: Re: Function attributes for make-docfile]
  2015-01-15  5:33               ` Stefan Monnier
@ 2015-01-16  4:50                 ` Paul Eggert
  0 siblings, 0 replies; 15+ messages in thread
From: Paul Eggert @ 2015-01-16  4:50 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Dmitry Antipov, Emacs development discussions

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

Dmitry Antipov wrote:
 > Why not use #pragma in the way similar to bytecode.c?

I'd rather not head down that direction, as it is ugly in a different way: seven 
lines' worth of preprocessor directives to silence one bogus diagnostic?!

 > '--enable-gcc-warnings --with-x-toolkit=no' build should have the
 > Fnext_read_file_uses_dialog_p-like issue with Fx_uses_old_gtk_dialog.
 > But I don't see it, and don't understand why

I don't know why either.  Apparently GCC does not do a perfect job of suggesting 
the attribute.

Stefan Monnier wrote:

> Easy, then: either you live with this warning, or you don't
> enable -Wsuggest-attribute=const.

Let's do the latter.  A couple of things.  First, I checked, and the attribute 
doesn't significantly help Emacs's performance, so it appears to be more trouble 
than it's worth for us.  Second, I audited the code and found five instances 
where we blindly followed GCC's advice and added ATTRIBUTE_CONST even though 
this was incorrect on some platforms; these mistakes suggest that we need to be 
more skeptical of GCC's diagnostics in this area on the rare occasions when we 
do use -Wsuggest-attribute=const.

I installed the attached patch to try to fix all this.

[-- Attachment #2: 0001-Give-up-on-Wsuggest-attribute-const.patch --]
[-- Type: text/x-patch, Size: 6761 bytes --]

From 64cdb38ed4483c089fff6f3ef6f1ad01f0a2a8d1 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 15 Jan 2015 20:37:05 -0800
Subject: [PATCH] Give up on -Wsuggest-attribute=const

The attribute doesn't help performance significantly, and the
warning seems to be more trouble than it's worth.  See the thread at:
http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00361.html
* configure.ac (WERROR_CFLAGS): Don't use -Wsuggest-attribute=const.
* lib-src/make-docfile.c (write_globals):
Remove special hack for Fnext_read_file_uses_dialog_p.
* src/decompress.c (Fzlib_available_p):
* src/gnutls.c (Fgnutls_available_p):
* src/gtkutil.h (xg_uses_old_file_dialog):
* src/xdisp.c (Ftool_bar_height):
* src/xmenu.c (popup_activated):
No longer const, since it's not const on at lest some
configurations, and we shouldn't lie to the compiler.
---
 ChangeLog              |  8 ++++++++
 configure.ac           |  5 ++++-
 lib-src/ChangeLog      |  6 ++++++
 lib-src/make-docfile.c | 12 ------------
 src/ChangeLog          | 11 +++++++++++
 src/decompress.c       |  3 +--
 src/gnutls.c           |  3 +--
 src/gtkutil.h          |  2 +-
 src/xdisp.c            |  3 +--
 src/xmenu.c            |  2 +-
 10 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cca9100..309b04f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2015-01-16  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Give up on -Wsuggest-attribute=const
+	The attribute doesn't help performance significantly, and the
+	warning seems to be more trouble than it's worth.  See the thread at:
+	http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00361.html
+	* configure.ac (WERROR_CFLAGS): Don't use -Wsuggest-attribute=const.
+
 2015-01-11  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Default to 'configure --enable-silent-rules'
diff --git a/configure.ac b/configure.ac
index 4cad214..9db4bde 100644
--- a/configure.ac
+++ b/configure.ac
@@ -892,6 +892,10 @@ else
   # Emacs's use of alloca inhibits protecting the stack.
   nw="$nw -Wstack-protector"
 
+  # Emacs's use of partly-const functions such as Fgnutls_available_p
+  # make this option problematic.
+  nw="$nw -Wsuggest-attribute=const"
+
   # Emacs's use of partly-pure functions such as CHECK_TYPE make this
   # option problematic.
   nw="$nw -Wsuggest-attribute=pure"
@@ -1974,7 +1978,6 @@ fi
 if test "$window_system" = none && test "$gl_gcc_warnings" = yes; then
    # Too many warnings for now.
    nw=
-   nw="$nw -Wsuggest-attribute=const"
    nw="$nw -Wsuggest-attribute=noreturn"
    gl_MANYWARN_COMPLEMENT([WARN_CFLAGS], [$WARN_CFLAGS], [$nw])
 
diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog
index e9205fd..7cbf327 100644
--- a/lib-src/ChangeLog
+++ b/lib-src/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-16  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Give up on -Wsuggest-attribute=const
+	* make-docfile.c (write_globals):
+	Remove special hack for Fnext_read_file_uses_dialog_p
+
 2015-01-13  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Don't say Fnext_read_file_uses_dialog_p is const
diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index 741fa4b..79d421a 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -729,18 +729,6 @@ write_globals (void)
 
 	  if (globals[i].flags & DEFUN_const)
 	    fputs (" ATTRIBUTE_CONST", stdout);
-	  else if (strcmp (globals[i].name, "Fnext_read_file_uses_dialog_p")
-		   == 0)
-	    {
-	      /* It would be nice to have a cleaner way to deal with this
-		 special hack.  */
-	      fputs (("\n"
-		      "#if ! (defined USE_GTK || defined USE_MOTIF \\\n"
-		      "       || defined HAVE_NS || defined HAVE_NTGUI)\n"
-		      "\tATTRIBUTE_CONST\n"
-		      "#endif\n"),
-		     stdout);
-	    }
 
 	  puts (";");
 	}
diff --git a/src/ChangeLog b/src/ChangeLog
index 40d8b26..ae634f3 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,14 @@
+2015-01-16  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Give up on -Wsuggest-attribute=const
+	* decompress.c (Fzlib_available_p):
+	* gnutls.c (Fgnutls_available_p):
+	* gtkutil.h (xg_uses_old_file_dialog):
+	* xdisp.c (Ftool_bar_height):
+	* xmenu.c (popup_activated):
+	No longer const, since it's not const on at lest some
+	configurations, and we shouldn't lie to the compiler.
+
 2015-01-15  Eli Zaretskii  <eliz@gnu.org>
 
 	* fileio.c: Include binary-io.h.
diff --git a/src/decompress.c b/src/decompress.c
index b78dace..b14f0a2 100644
--- a/src/decompress.c
+++ b/src/decompress.c
@@ -88,8 +88,7 @@ unwind_decompress (void *ddata)
 }
 
 DEFUN ("zlib-available-p", Fzlib_available_p, Szlib_available_p, 0, 0, 0,
-       doc: /* Return t if zlib decompression is available in this instance of Emacs.  */
-       attributes: const)
+       doc: /* Return t if zlib decompression is available in this instance of Emacs.  */)
      (void)
 {
 #ifdef WINDOWSNT
diff --git a/src/gnutls.c b/src/gnutls.c
index 909542f..35f0eb4 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1619,8 +1619,7 @@ This function may also return `gnutls-e-again', or
 #endif	/* HAVE_GNUTLS */
 
 DEFUN ("gnutls-available-p", Fgnutls_available_p, Sgnutls_available_p, 0, 0, 0,
-       doc: /* Return t if GnuTLS is available in this instance of Emacs.  */
-       attributes: const)
+       doc: /* Return t if GnuTLS is available in this instance of Emacs.  */)
      (void)
 {
 #ifdef HAVE_GNUTLS
diff --git a/src/gtkutil.h b/src/gtkutil.h
index 7d712c9..0ac49ca 100644
--- a/src/gtkutil.h
+++ b/src/gtkutil.h
@@ -78,7 +78,7 @@ typedef struct xg_menu_item_cb_data_
 
 } xg_menu_item_cb_data;
 
-extern bool xg_uses_old_file_dialog (void) ATTRIBUTE_CONST;
+extern bool xg_uses_old_file_dialog (void);
 
 extern char *xg_get_file_name (struct frame *f,
                                char *prompt,
diff --git a/src/xdisp.c b/src/xdisp.c
index 041a022..f006f8e 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -12276,8 +12276,7 @@ DEFUN ("tool-bar-height", Ftool_bar_height, Stool_bar_height,
        0, 2, 0,
        doc: /* Return the number of lines occupied by the tool bar of FRAME.
 If FRAME is nil or omitted, use the selected frame.  Optional argument
-PIXELWISE non-nil means return the height of the tool bar in pixels.  */
-       attributes: const)
+PIXELWISE non-nil means return the height of the tool bar in pixels.  */)
   (Lisp_Object frame, Lisp_Object pixelwise)
 {
   int height = 0;
diff --git a/src/xmenu.c b/src/xmenu.c
index 9063a8a..fdf1f6f 100644
--- a/src/xmenu.c
+++ b/src/xmenu.c
@@ -2288,7 +2288,7 @@ x_menu_show (struct frame *f, int x, int y, int menuflags,
 /* Detect if a dialog or menu has been posted.  MSDOS has its own
    implementation on msdos.c.  */
 
-int ATTRIBUTE_CONST
+int
 popup_activated (void)
 {
   return popup_activated_flag;
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2015-01-16  4:50 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-12  4:09 Function attributes for make-docfile Dmitry Antipov
2015-01-12  5:33 ` Stefan Monnier
2015-01-12  5:49 ` Paul Eggert
2015-01-12 12:28   ` #2 [Was: Re: Function attributes for make-docfile] Dmitry Antipov
2015-01-13  7:29     ` Paul Eggert
2015-01-13 10:21       ` Dmitry Antipov
2015-01-13 17:43         ` Paul Eggert
2015-01-13 19:45       ` Stefan Monnier
2015-01-13 23:26         ` Paul Eggert
2015-01-14 17:49           ` Stefan Monnier
2015-01-14 19:44             ` Paul Eggert
2015-01-15  3:27               ` Dmitry Antipov
2015-01-15  3:54                 ` Dmitry Antipov
2015-01-15  5:33               ` Stefan Monnier
2015-01-16  4:50                 ` Paul Eggert

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.