[PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Kaveh R. GHAZI]

This patch addresses problems in achieving clean -Wcast-qual results.
The issue is that there are certain legitimate situations where it is
impossible in C to avoid casting away const-ness.  Such examples IMHO
include:

1.  Passing const argv strings to execv-like functions.

2.  Initializing const allocated objects.

3.  Free'ing const objects from #2.

(It's been suggested that adding a specialized const_free(const void*)
to libiberty could hide casting away const-ness.  However that only
solves one of the above three cases.)

So I added a mechanism to system.h that uses a union cast to get
around -Wcast-qual.  I named the macro CONST_CAST(X) in honor of the
C++ operator of similar utility.

I believe all of the cases where I used it below fall into one or more
of the above three categories.

Tested on sparc-sun-solaris2.10, no regressions.  I used cc for stage1
to make sure that the non-gcc case works.  I also tested it with
gcc-3.4.6 to make sure that the union cast worked for older gcc
versions.

Okay for mainline?  (If someone approves the system.h mechanism, I'll
install the rest as obvious.)

		Thanks,
		--Kaveh


2007-07-24  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

	* system.h (CONST_CAST): New.
	* c-decl.c (c_make_fname_decl): Use it.
	* c-lex.c (cb_ident, lex_string): Likewise.
	* c-typeck.c (free_all_tagged_tu_seen_up_to): Likewise.
	* gcc.c (set_spec, read_specs, for_each_path, execute, do_spec_1,
	give_switch, set_multilib_dir): Likewise.
	* gengtype-parse.c (string_seq, typedef_name): Likewise.
	* passes.c (execute_one_pass): Likewise.
	* prefix.c (update_path): Likewise.
	* pretty-print.c (pp_base_destroy_prefix): Likewise.
	* tree.c (build_string): Likewise.

cp:
	* call.c (name_as_c_string): Use CONST_CAST.
	* decl.c (build_decl): Likewise.
	* parser.c (cp_parser_string_literal): Likewise.

fortran:
	* gfortranspec.c (lang_specific_driver): Use CONST_CAST.
	* options.c (gfc_post_options): Likewise.
	* parse.c (parse_omp_structured_block): Likewise.
	* st.c (gfc_free_statement): Likewise.

java:
	* jcf-parse.c (read_class, java_parse_file): Use CONST_CAST.
	* jcf.h (JCF_FINISH): Likewise.

diff -rup orig/egcc-SVN20070721/gcc/system.h egcc-SVN20070721/gcc/system.h
--- orig/egcc-SVN20070721/gcc/system.h	2007-05-03 03:14:36.000000000 -0400
+++ egcc-SVN20070721/gcc/system.h	2007-07-24 20:43:38.775320815 -0400
@@ -767,4 +767,23 @@ extern void fancy_abort (const char *, i

 #endif /* GCC >= 3.0 */

+/* This macro allows casting away const-ness to pass -Wcast-qual
+   warnings.  DO NOT USE THIS UNLESS YOU REALLY HAVE TO!  It should
+   only be used in certain specific cases.  One valid case is where
+   the C standard definitions or prototypes force you to.  E.g. if you
+   need to free a const object, or if you pass a const string to
+   execv, et al.  Another valid use would be in an allocation function
+   that creates const objects that need to be initialized.  Most other
+   cases should be viewed with extreme caution.  */
+#ifdef __GNUC__
+union gcc_constcast
+{
+  const void *cv;
+  void *v;
+};
+#define CONST_CAST(X) ((__extension__(union gcc_constcast)(const void *)(X)).v)
+#else
+#define CONST_CAST(X) ((void*)(X))
+#endif
+
 #endif /* ! GCC_SYSTEM_H */
diff -rup orig/egcc-SVN20070721/gcc/c-decl.c egcc-SVN20070721/gcc/c-decl.c
--- orig/egcc-SVN20070721/gcc/c-decl.c	2007-07-13 12:23:51.000000000 -0400
+++ egcc-SVN20070721/gcc/c-decl.c	2007-07-24 19:03:34.873245149 -0400
@@ -2812,7 +2812,7 @@ c_make_fname_decl (tree id, int type_dep
   DECL_ARTIFICIAL (decl) = 1;

   init = build_string (length + 1, name);
-  free ((char *) name);
+  free (CONST_CAST (name));
   TREE_TYPE (init) = type;
   DECL_INITIAL (decl) = init;

diff -rup orig/egcc-SVN20070721/gcc/c-lex.c egcc-SVN20070721/gcc/c-lex.c
--- orig/egcc-SVN20070721/gcc/c-lex.c	2007-07-24 18:40:58.373062728 -0400
+++ egcc-SVN20070721/gcc/c-lex.c	2007-07-24 19:05:18.535878607 -0400
@@ -187,7 +187,7 @@ cb_ident (cpp_reader * ARG_UNUSED (pfile
       if (cpp_interpret_string (pfile, str, 1, &cstr, false))
 	{
 	  ASM_OUTPUT_IDENT (asm_out_file, (const char *) cstr.text);
-	  free ((void *) cstr.text);
+	  free (CONST_CAST (cstr.text));
 	}
     }
 #endif
@@ -812,7 +812,7 @@ lex_string (const cpp_token *tok, tree *
       (parse_in, strs, concats + 1, &istr, wide))
     {
       value = build_string (istr.len, (const char *) istr.text);
-      free ((void *) istr.text);
+      free (CONST_CAST (istr.text));

       if (c_lex_string_translate == -1)
 	{
@@ -833,7 +833,7 @@ lex_string (const cpp_token *tok, tree *
 	      *valp = build_string (istr.len, (const char *) istr.text);
 	      valp = &TREE_CHAIN (*valp);
 	    }
-	  free ((void *) istr.text);
+	  free (CONST_CAST (istr.text));
 	}
     }
   else
diff -rup orig/egcc-SVN20070721/gcc/c-typeck.c egcc-SVN20070721/gcc/c-typeck.c
--- orig/egcc-SVN20070721/gcc/c-typeck.c	2007-07-24 18:40:58.444523088 -0400
+++ egcc-SVN20070721/gcc/c-typeck.c	2007-07-24 19:02:51.153971227 -0400
@@ -1027,7 +1027,7 @@ free_all_tagged_tu_seen_up_to (const str
       const struct tagged_tu_seen_cache *const tu1
 	= (const struct tagged_tu_seen_cache *) tu;
       tu = tu1->next;
-      free ((void *)tu1);
+      free (CONST_CAST (tu1));
     }
   tagged_tu_seen_base = tu_til;
 }
diff -rup orig/egcc-SVN20070721/gcc/cp/call.c egcc-SVN20070721/gcc/cp/call.c
--- orig/egcc-SVN20070721/gcc/cp/call.c	2007-07-05 23:03:38.000000000 -0400
+++ egcc-SVN20070721/gcc/cp/call.c	2007-07-24 19:47:59.090395675 -0400
@@ -5367,7 +5367,7 @@ name_as_c_string (tree name, tree type,
   if (IDENTIFIER_CTOR_OR_DTOR_P (name))
     {
       pretty_name
-	= (char *) IDENTIFIER_POINTER (constructor_name (type));
+	= (char *) CONST_CAST (IDENTIFIER_POINTER (constructor_name (type)));
       /* For a destructor, add the '~'.  */
       if (name == complete_dtor_identifier
 	  || name == base_dtor_identifier
@@ -5388,7 +5388,7 @@ name_as_c_string (tree name, tree type,
       *free_p = true;
     }
   else
-    pretty_name = (char *) IDENTIFIER_POINTER (name);
+    pretty_name = (char *) CONST_CAST (IDENTIFIER_POINTER (name));

   return pretty_name;
 }
diff -rup orig/egcc-SVN20070721/gcc/cp/decl.c egcc-SVN20070721/gcc/cp/decl.c
--- orig/egcc-SVN20070721/gcc/cp/decl.c	2007-07-24 18:40:58.457872625 -0400
+++ egcc-SVN20070721/gcc/cp/decl.c	2007-07-24 19:49:09.242333435 -0400
@@ -3369,7 +3369,7 @@ cp_make_fname_decl (tree id, int type_de
   tree decl = build_decl (VAR_DECL, id, type);

   if (name)
-    free ((char *) name);
+    free (CONST_CAST (name));

   /* As we're using pushdecl_with_scope, we must set the context.  */
   DECL_CONTEXT (decl) = current_function_decl;
diff -rup orig/egcc-SVN20070721/gcc/cp/parser.c egcc-SVN20070721/gcc/cp/parser.c
--- orig/egcc-SVN20070721/gcc/cp/parser.c	2007-07-13 12:23:18.000000000 -0400
+++ egcc-SVN20070721/gcc/cp/parser.c	2007-07-24 19:55:10.357909872 -0400
@@ -2919,8 +2919,8 @@ cp_parser_string_literal (cp_parser *par
   if ((translate ? cpp_interpret_string : cpp_interpret_string_notranslate)
       (parse_in, strs, count, &istr, wide))
     {
-      value = build_string (istr.len, (char *)istr.text);
-      free ((void *)istr.text);
+      value = build_string (istr.len, (const char *)istr.text);
+      free (CONST_CAST (istr.text));

       TREE_TYPE (value) = wide ? wchar_array_type_node : char_array_type_node;
       value = fix_string_type (value);
diff -rup orig/egcc-SVN20070721/gcc/fortran/gfortranspec.c egcc-SVN20070721/gcc/fortran/gfortranspec.c
--- orig/egcc-SVN20070721/gcc/fortran/gfortranspec.c	2007-06-27 19:45:34.000000000 -0400
+++ egcc-SVN20070721/gcc/fortran/gfortranspec.c	2007-07-24 19:06:28.868883584 -0400
@@ -303,7 +303,7 @@ lang_specific_driver (int *in_argc, cons
   g77_xargc = argc;
   g77_xargv = argv;
   g77_newargc = 0;
-  g77_newargv = (const char **) argv;
+  g77_newargv = (const char **) CONST_CAST (argv);

   /* First pass through arglist.

diff -rup orig/egcc-SVN20070721/gcc/fortran/options.c egcc-SVN20070721/gcc/fortran/options.c
--- orig/egcc-SVN20070721/gcc/fortran/options.c	2007-07-15 23:02:34.000000000 -0400
+++ egcc-SVN20070721/gcc/fortran/options.c	2007-07-24 19:44:53.904878737 -0400
@@ -240,7 +240,7 @@ gfc_post_options (const char **pfilename
     gfc_add_include_path (".", true);

   if (canon_source_file != gfc_source_file)
-    gfc_free ((void *) canon_source_file);
+    gfc_free (CONST_CAST (canon_source_file));

   /* Decide which form the file will be read in as.  */

diff -rup orig/egcc-SVN20070721/gcc/fortran/parse.c egcc-SVN20070721/gcc/fortran/parse.c
--- orig/egcc-SVN20070721/gcc/fortran/parse.c	2007-07-14 23:02:20.000000000 -0400
+++ egcc-SVN20070721/gcc/fortran/parse.c	2007-07-24 19:43:56.383053875 -0400
@@ -2609,7 +2609,7 @@ parse_omp_structured_block (gfc_statemen
 	      && strcmp (cp->ext.omp_name, new_st.ext.omp_name) != 0))
 	gfc_error ("Name after !$omp critical and !$omp end critical does "
 		   "not match at %C");
-      gfc_free ((char *) new_st.ext.omp_name);
+      gfc_free (CONST_CAST (new_st.ext.omp_name));
       break;
     case EXEC_OMP_END_SINGLE:
       cp->ext.omp_clauses->lists[OMP_LIST_COPYPRIVATE]
diff -rup orig/egcc-SVN20070721/gcc/fortran/st.c egcc-SVN20070721/gcc/fortran/st.c
--- orig/egcc-SVN20070721/gcc/fortran/st.c	2007-01-20 20:01:25.000000000 -0500
+++ egcc-SVN20070721/gcc/fortran/st.c	2007-07-24 19:42:59.565302777 -0400
@@ -174,7 +174,7 @@ gfc_free_statement (gfc_code *p)
       break;

     case EXEC_OMP_CRITICAL:
-      gfc_free ((char *) p->ext.omp_name);
+      gfc_free (CONST_CAST (p->ext.omp_name));
       break;

     case EXEC_OMP_FLUSH:
diff -rup orig/egcc-SVN20070721/gcc/gcc.c egcc-SVN20070721/gcc/gcc.c
--- orig/egcc-SVN20070721/gcc/gcc.c	2007-06-27 19:46:07.000000000 -0400
+++ egcc-SVN20070721/gcc/gcc.c	2007-07-24 19:00:24.586507520 -0400
@@ -1877,7 +1877,7 @@ set_spec (const char *name, const char *

   /* Free the old spec.  */
   if (old_spec && sl->alloc_p)
-    free ((void *) old_spec);
+    free (CONST_CAST(old_spec));

   sl->alloc_p = 1;
 }
@@ -2182,7 +2182,7 @@ read_specs (const char *filename, int ma

 	      set_spec (p2, *(sl->ptr_spec));
 	      if (sl->alloc_p)
-		free ((void *) *(sl->ptr_spec));
+		free (CONST_CAST (*(sl->ptr_spec)));

 	      *(sl->ptr_spec) = "";
 	      sl->alloc_p = 0;
@@ -2532,18 +2532,18 @@ for_each_path (const struct path_prefix
 	 Don't repeat any we have already seen.  */
       if (multi_dir)
 	{
-	  free ((char *) multi_dir);
+	  free (CONST_CAST (multi_dir));
 	  multi_dir = NULL;
-	  free ((char *) multi_suffix);
+	  free (CONST_CAST (multi_suffix));
 	  multi_suffix = machine_suffix;
-	  free ((char *) just_multi_suffix);
+	  free (CONST_CAST (just_multi_suffix));
 	  just_multi_suffix = just_machine_suffix;
 	}
       else
 	skip_multi_dir = true;
       if (multi_os_dir)
 	{
-	  free ((char *) multi_os_dir);
+	  free (CONST_CAST (multi_os_dir));
 	  multi_os_dir = NULL;
 	}
       else
@@ -2552,12 +2552,12 @@ for_each_path (const struct path_prefix

   if (multi_dir)
     {
-      free ((char *) multi_dir);
-      free ((char *) multi_suffix);
-      free ((char *) just_multi_suffix);
+      free (CONST_CAST (multi_dir));
+      free (CONST_CAST (multi_suffix));
+      free (CONST_CAST (just_multi_suffix));
     }
   if (multi_os_dir)
-    free ((char *) multi_os_dir);
+    free (CONST_CAST (multi_os_dir));
   if (ret != path)
     free (path);
   return ret;
@@ -2964,7 +2964,7 @@ execute (void)
       errmsg = pex_run (pex,
 			((i + 1 == n_commands ? PEX_LAST : 0)
 			 | (string == commands[i].prog ? PEX_SEARCH : 0)),
-			string, (char * const *) commands[i].argv,
+			string, (char * const *) CONST_CAST (commands[i].argv),
 			NULL, NULL, &err);
       if (errmsg != NULL)
 	{
@@ -2978,7 +2978,7 @@ execute (void)
 	}

       if (string != commands[i].prog)
-	free ((void *) string);
+	free (CONST_CAST (string));
     }

   execution_count++;
@@ -5042,7 +5042,7 @@ do_spec_1 (const char *spec, int inswitc
                   for (i = 0, j = 0; i < max; i++)
                     if (outfiles[i])
                       {
-                        argv[j] = (char *) outfiles[i];
+                        argv[j] = (char *) CONST_CAST (outfiles[i]);
                         j++;
                       }
                   argv[j] = NULL;
@@ -6018,13 +6018,13 @@ give_switch (int switchnum, int omit_fir
 	      while (length-- && !IS_DIR_SEPARATOR (arg[length]))
 		if (arg[length] == '.')
 		  {
-		    ((char *)arg)[length] = 0;
+		    ((char *)CONST_CAST(arg))[length] = 0;
 		    dot = 1;
 		    break;
 		  }
 	      do_spec_1 (arg, 1, NULL);
 	      if (dot)
-		((char *)arg)[length] = '.';
+		((char *)CONST_CAST(arg))[length] = '.';
 	      do_spec_1 (suffix_subst, 1, NULL);
 	    }
 	  else
@@ -7477,7 +7477,7 @@ set_multilib_dir (void)
   if (multilib_dir == NULL && multilib_os_dir != NULL
       && strcmp (multilib_os_dir, ".") == 0)
     {
-      free ((char *) multilib_os_dir);
+      free (CONST_CAST (multilib_os_dir));
       multilib_os_dir = NULL;
     }
   else if (multilib_dir != NULL && multilib_os_dir == NULL)
diff -rup orig/egcc-SVN20070721/gcc/gengtype-parse.c egcc-SVN20070721/gcc/gengtype-parse.c
--- orig/egcc-SVN20070721/gcc/gengtype-parse.c	2007-03-26 20:02:43.000000000 -0500
+++ egcc-SVN20070721/gcc/gengtype-parse.c	2007-07-24 20:20:11.483830545 -0400
@@ -198,9 +198,9 @@ string_seq (void)

       l1 = strlen (s1);
       l2 = strlen (s2);
-      buf = XRESIZEVEC (char, s1, l1 + l2 + 1);
+      buf = XRESIZEVEC (char, CONST_CAST(s1), l1 + l2 + 1);
       memcpy (buf + l1, s2, l2 + 1);
-      XDELETE (s2);
+      XDELETE (CONST_CAST (s2));
       s1 = buf;
     }
   return s1;
@@ -222,8 +222,8 @@ typedef_name (void)
       c2 = require (ID);
       require (')');
       r = concat ("VEC_", c1, "_", c2, (char *)0);
-      free ((void *)c1);
-      free ((void *)c2);
+      free (CONST_CAST (c1));
+      free (CONST_CAST (c2));
       return r;
     }
   else
diff -rup orig/egcc-SVN20070721/gcc/java/jcf-parse.c egcc-SVN20070721/gcc/java/jcf-parse.c
--- orig/egcc-SVN20070721/gcc/java/jcf-parse.c	2007-07-24 18:40:58.550054291 -0400
+++ egcc-SVN20070721/gcc/java/jcf-parse.c	2007-07-24 19:36:28.693654939 -0400
@@ -1302,7 +1302,7 @@ read_class (tree name)
       if (path_name == 0)
 	return 0;
       else
-	free((char *) path_name);
+	free(CONST_CAST (path_name));
     }

   current_jcf = jcf;
@@ -1784,7 +1784,7 @@ java_parse_file (int set_yydebug ATTRIBU
       file_list = list;
     }
   else
-    list = (char *) main_input_filename;
+    list = (char *) CONST_CAST (main_input_filename);

   while (list)
     {
diff -rup orig/egcc-SVN20070721/gcc/java/jcf.h egcc-SVN20070721/gcc/java/jcf.h
--- orig/egcc-SVN20070721/gcc/java/jcf.h	2007-01-13 20:02:03.000000000 -0500
+++ egcc-SVN20070721/gcc/java/jcf.h	2007-07-24 19:33:07.494933312 -0400
@@ -165,8 +165,8 @@ typedef struct JCF GTY(()) {
 #define JCF_FINISH(JCF) { \
   CPOOL_FINISH(&(JCF)->cpool); \
   if ((JCF)->buffer) free ((JCF)->buffer); \
-  if ((JCF)->filename) free ((char *) (JCF)->filename); \
-  if ((JCF)->classname) free ((char *) (JCF)->classname); \
+  if ((JCF)->filename) free (CONST_CAST ((JCF)->filename)); \
+  if ((JCF)->classname) free (CONST_CAST ((JCF)->classname)); \
   (JCF)->finished = 1; }

 #define CPOOL_INIT(CPOOL) \
diff -rup orig/egcc-SVN20070721/gcc/passes.c egcc-SVN20070721/gcc/passes.c
--- orig/egcc-SVN20070721/gcc/passes.c	2007-07-13 12:23:53.000000000 -0400
+++ egcc-SVN20070721/gcc/passes.c	2007-07-24 19:02:17.091278504 -0400
@@ -1153,7 +1153,7 @@ execute_one_pass (struct tree_opt_pass *
   /* Flush and close dump file.  */
   if (dump_file_name)
     {
-      free ((char *) dump_file_name);
+      free (CONST_CAST (dump_file_name));
       dump_file_name = NULL;
     }

diff -rup orig/egcc-SVN20070721/gcc/prefix.c egcc-SVN20070721/gcc/prefix.c
--- orig/egcc-SVN20070721/gcc/prefix.c	2006-01-23 00:24:19.000000000 -0500
+++ egcc-SVN20070721/gcc/prefix.c	2007-07-24 20:05:13.775595567 -0400
@@ -267,7 +267,7 @@ update_path (const char *path, const cha

       result = concat (key, &path[len], NULL);
       if (free_key)
-	free ((char *) key);
+	free (CONST_CAST (key));
       result = translate_name (result);
     }
   else
diff -rup orig/egcc-SVN20070721/gcc/pretty-print.c egcc-SVN20070721/gcc/pretty-print.c
--- orig/egcc-SVN20070721/gcc/pretty-print.c	2006-01-31 15:37:18.000000000 -0500
+++ egcc-SVN20070721/gcc/pretty-print.c	2007-07-24 20:04:22.383432306 -0400
@@ -634,7 +634,7 @@ pp_base_destroy_prefix (pretty_printer *
 {
   if (pp->prefix != NULL)
     {
-      free ((char *) pp->prefix);
+      free (CONST_CAST (pp->prefix));
       pp->prefix = NULL;
     }
 }
diff -rup orig/egcc-SVN20070721/gcc/tree.c egcc-SVN20070721/gcc/tree.c
--- orig/egcc-SVN20070721/gcc/tree.c	2007-07-24 18:40:58.728953172 -0400
+++ egcc-SVN20070721/gcc/tree.c	2007-07-24 20:39:42.541854067 -0400
@@ -1177,8 +1177,8 @@ build_string (int len, const char *str)
   TREE_CONSTANT (s) = 1;
   TREE_INVARIANT (s) = 1;
   TREE_STRING_LENGTH (s) = len;
-  memcpy ((char *) TREE_STRING_POINTER (s), str, len);
-  ((char *) TREE_STRING_POINTER (s))[len] = '\0';
+  memcpy (CONST_CAST (TREE_STRING_POINTER (s)), str, len);
+  ((char *) CONST_CAST (TREE_STRING_POINTER (s)))[len] = '\0';

   return s;
 }

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Richard Guenther]

On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> This patch addresses problems in achieving clean -Wcast-qual results.
> The issue is that there are certain legitimate situations where it is
> impossible in C to avoid casting away const-ness.  Such examples IMHO
> include:
>
> 1.  Passing const argv strings to execv-like functions.
>
> 2.  Initializing const allocated objects.
>
> 3.  Free'ing const objects from #2.
>
> (It's been suggested that adding a specialized const_free(const void*)
> to libiberty could hide casting away const-ness.  However that only
> solves one of the above three cases.)
>
> So I added a mechanism to system.h that uses a union cast to get
> around -Wcast-qual.  I named the macro CONST_CAST(X) in honor of the
> C++ operator of similar utility.
>
> I believe all of the cases where I used it below fall into one or more
> of the above three categories.
>
> Tested on sparc-sun-solaris2.10, no regressions.  I used cc for stage1
> to make sure that the non-gcc case works.  I also tested it with
> gcc-3.4.6 to make sure that the union cast worked for older gcc
> versions.
>
> Okay for mainline?  (If someone approves the system.h mechanism, I'll
> install the rest as obvious.)

> +union gcc_constcast
> +{
> +  const void *cv;
> +  void *v;
> +};
> +#define CONST_CAST(X) ((__extension__(union gcc_constcast)(const void *)(X)).v)

Can you explain in how this is better than using an explicit cast to
the non-const
version like in memcpy ((char *)x, ...)?  At least it cannot detect
wrongly typed
arguments anymore due to the use of void*.  ATM we could do the following:

#define CONST_CAST(X) ((__extension__((union { __typeof__(X) _q; void
*v; })(X)).v)

and I'd suggest to create a __typeof__ that strips CV qualifiers to be
able to get
rid of the void* member in the union as well.

Richard.

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Kaveh R. GHAZI]

On Fri, 27 Jul 2007, Richard Guenther wrote:

> On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> > +union gcc_constcast
> > +{
> > +  const void *cv;
> > +  void *v;
> > +};
> > +#define CONST_CAST(X) ((__extension__(union gcc_constcast)(const void *)(X)).v)
>
> Can you explain in how this is better than using an explicit cast to the
> non-const version like in memcpy ((char *)x, ...)?

Sure.  Besides my main point which is silencing -Wcast-qual warnings, the
CONST_CAST macro is grep-able.  I.e. you can search for the few places
where it's used and audit if they are correctly applied.  You cannot do
that with regular C-style casts.  Another reason is that with C-style
casts there isn't any indication of why it's there.  The CONST_CAST macro
comments itself to someone reading the code.  (These are some of the
rationales for the creation of C++'s cast operators.)  The macro isn't
type-safe like the C++ operator is, but it's a poor man's C option. :-)

I agree a new __typeof (__main_typeof?) that only strips the qualifiers
would be a good refinement for type-safety.  But that's something which
can be altered in the macro definition as a followup if/when someone
implements it.



> At least it cannot detect wrongly typed arguments anymore due to the use
> of void*.  ATM we could do the following:
>
> #define CONST_CAST(X) ((__extension__((union { __typeof__(X) _q; void
> *v; })(X)).v)


This version uses the macro definition you suggested in system.h.  The
rest of the patch is unchanged.  Okay for mainline?

		Thanks,
		--Kaveh


2007-07-24  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

	* system.h (CONST_CAST): New.
	* c-decl.c (c_make_fname_decl): Use it.
	* c-lex.c (cb_ident, lex_string): Likewise.
	* c-typeck.c (free_all_tagged_tu_seen_up_to): Likewise.
	* gcc.c (set_spec, read_specs, for_each_path, execute, do_spec_1,
	give_switch, set_multilib_dir): Likewise.
	* gengtype-parse.c (string_seq, typedef_name): Likewise.
	* passes.c (execute_one_pass): Likewise.
	* prefix.c (update_path): Likewise.
	* pretty-print.c (pp_base_destroy_prefix): Likewise.
	* tree.c (build_string): Likewise.

cp:
	* call.c (name_as_c_string): Use CONST_CAST.
	* decl.c (build_decl): Likewise.
	* parser.c (cp_parser_string_literal): Likewise.

fortran:
	* gfortranspec.c (lang_specific_driver): Use CONST_CAST.
	* options.c (gfc_post_options): Likewise.
	* parse.c (parse_omp_structured_block): Likewise.
	* st.c (gfc_free_statement): Likewise.

java:
	* jcf-parse.c (read_class, java_parse_file): Use CONST_CAST.
	* jcf.h (JCF_FINISH): Likewise.

diff -rup orig/egcc-SVN20070727/gcc/system.h egcc-SVN20070727/gcc/system.h
--- orig/egcc-SVN20070727/gcc/system.h	2007-07-26 23:04:08.000000000 -0400
+++ egcc-SVN20070727/gcc/system.h	2007-07-27 12:32:41.799916226 -0400
@@ -766,4 +766,18 @@ extern void fancy_abort (const char *, i

 #endif /* GCC >= 3.0 */

+/* This macro allows casting away const-ness to pass -Wcast-qual
+   warnings.  DO NOT USE THIS UNLESS YOU REALLY HAVE TO!  It should
+   only be used in certain specific cases.  One valid case is where
+   the C standard definitions or prototypes force you to.  E.g. if you
+   need to free a const object, or if you pass a const string to
+   execv, et al.  Another valid use would be in an allocation function
+   that creates const objects that need to be initialized.  Most other
+   cases should be viewed with extreme caution.  */
+#ifdef __GNUC__
+#define CONST_CAST(X) ((__extension__(union {__typeof(X)_q; void *_v;})(X))._v)
+#else
+#define CONST_CAST(X) ((void*)(X))
+#endif
+
 #endif /* ! GCC_SYSTEM_H */
diff -rup orig/egcc-SVN20070727/gcc/c-decl.c egcc-SVN20070727/gcc/c-decl.c
--- orig/egcc-SVN20070727/gcc/c-decl.c	2007-07-26 23:03:51.000000000 -0400
+++ egcc-SVN20070727/gcc/c-decl.c	2007-07-27 12:22:09.221318916 -0400
@@ -2811,7 +2811,7 @@ c_make_fname_decl (tree id, int type_dep
   DECL_ARTIFICIAL (decl) = 1;

   init = build_string (length + 1, name);
-  free ((char *) name);
+  free (CONST_CAST (name));
   TREE_TYPE (init) = type;
   DECL_INITIAL (decl) = init;

diff -rup orig/egcc-SVN20070727/gcc/c-lex.c egcc-SVN20070727/gcc/c-lex.c
--- orig/egcc-SVN20070727/gcc/c-lex.c	2007-07-26 23:03:30.000000000 -0400
+++ egcc-SVN20070727/gcc/c-lex.c	2007-07-27 12:22:09.224377056 -0400
@@ -186,7 +186,7 @@ cb_ident (cpp_reader * ARG_UNUSED (pfile
       if (cpp_interpret_string (pfile, str, 1, &cstr, false))
 	{
 	  ASM_OUTPUT_IDENT (asm_out_file, (const char *) cstr.text);
-	  free ((void *) cstr.text);
+	  free (CONST_CAST (cstr.text));
 	}
     }
 #endif
@@ -811,7 +811,7 @@ lex_string (const cpp_token *tok, tree *
       (parse_in, strs, concats + 1, &istr, wide))
     {
       value = build_string (istr.len, (const char *) istr.text);
-      free ((void *) istr.text);
+      free (CONST_CAST (istr.text));

       if (c_lex_string_translate == -1)
 	{
@@ -832,7 +832,7 @@ lex_string (const cpp_token *tok, tree *
 	      *valp = build_string (istr.len, (const char *) istr.text);
 	      valp = &TREE_CHAIN (*valp);
 	    }
-	  free ((void *) istr.text);
+	  free (CONST_CAST (istr.text));
 	}
     }
   else
diff -rup orig/egcc-SVN20070727/gcc/c-typeck.c egcc-SVN20070727/gcc/c-typeck.c
--- orig/egcc-SVN20070727/gcc/c-typeck.c	2007-07-26 23:03:57.000000000 -0400
+++ egcc-SVN20070727/gcc/c-typeck.c	2007-07-27 12:22:09.235239783 -0400
@@ -1026,7 +1026,7 @@ free_all_tagged_tu_seen_up_to (const str
       const struct tagged_tu_seen_cache *const tu1
 	= (const struct tagged_tu_seen_cache *) tu;
       tu = tu1->next;
-      free ((void *)tu1);
+      free (CONST_CAST (tu1));
     }
   tagged_tu_seen_base = tu_til;
 }
diff -rup orig/egcc-SVN20070727/gcc/cp/call.c egcc-SVN20070727/gcc/cp/call.c
--- orig/egcc-SVN20070727/gcc/cp/call.c	2007-07-05 23:03:38.000000000 -0400
+++ egcc-SVN20070727/gcc/cp/call.c	2007-07-27 12:22:09.243329235 -0400
@@ -5367,7 +5367,7 @@ name_as_c_string (tree name, tree type,
   if (IDENTIFIER_CTOR_OR_DTOR_P (name))
     {
       pretty_name
-	= (char *) IDENTIFIER_POINTER (constructor_name (type));
+	= (char *) CONST_CAST (IDENTIFIER_POINTER (constructor_name (type)));
       /* For a destructor, add the '~'.  */
       if (name == complete_dtor_identifier
 	  || name == base_dtor_identifier
@@ -5388,7 +5388,7 @@ name_as_c_string (tree name, tree type,
       *free_p = true;
     }
   else
-    pretty_name = (char *) IDENTIFIER_POINTER (name);
+    pretty_name = (char *) CONST_CAST (IDENTIFIER_POINTER (name));

   return pretty_name;
 }
diff -rup orig/egcc-SVN20070727/gcc/cp/decl.c egcc-SVN20070727/gcc/cp/decl.c
--- orig/egcc-SVN20070727/gcc/cp/decl.c	2007-07-25 15:13:27.000000000 -0400
+++ egcc-SVN20070727/gcc/cp/decl.c	2007-07-27 12:22:09.259396354 -0400
@@ -3369,7 +3369,7 @@ cp_make_fname_decl (tree id, int type_de
   tree decl = build_decl (VAR_DECL, id, type);

   if (name)
-    free ((char *) name);
+    free (CONST_CAST (name));

   /* As we're using pushdecl_with_scope, we must set the context.  */
   DECL_CONTEXT (decl) = current_function_decl;
diff -rup orig/egcc-SVN20070727/gcc/cp/parser.c egcc-SVN20070727/gcc/cp/parser.c
--- orig/egcc-SVN20070727/gcc/cp/parser.c	2007-07-25 15:52:37.000000000 -0400
+++ egcc-SVN20070727/gcc/cp/parser.c	2007-07-27 12:22:09.285787196 -0400
@@ -2919,8 +2919,8 @@ cp_parser_string_literal (cp_parser *par
   if ((translate ? cpp_interpret_string : cpp_interpret_string_notranslate)
       (parse_in, strs, count, &istr, wide))
     {
-      value = build_string (istr.len, (char *)istr.text);
-      free ((void *)istr.text);
+      value = build_string (istr.len, (const char *)istr.text);
+      free (CONST_CAST (istr.text));

       TREE_TYPE (value) = wide ? wchar_array_type_node : char_array_type_node;
       value = fix_string_type (value);
diff -rup orig/egcc-SVN20070727/gcc/fortran/gfortranspec.c egcc-SVN20070727/gcc/fortran/gfortranspec.c
--- orig/egcc-SVN20070727/gcc/fortran/gfortranspec.c	2007-06-27 19:45:34.000000000 -0400
+++ egcc-SVN20070727/gcc/fortran/gfortranspec.c	2007-07-27 12:22:09.288230636 -0400
@@ -303,7 +303,7 @@ lang_specific_driver (int *in_argc, cons
   g77_xargc = argc;
   g77_xargv = argv;
   g77_newargc = 0;
-  g77_newargv = (const char **) argv;
+  g77_newargv = (const char **) CONST_CAST (argv);

   /* First pass through arglist.

diff -rup orig/egcc-SVN20070727/gcc/fortran/options.c egcc-SVN20070727/gcc/fortran/options.c
--- orig/egcc-SVN20070727/gcc/fortran/options.c	2007-07-15 23:02:34.000000000 -0400
+++ egcc-SVN20070727/gcc/fortran/options.c	2007-07-27 12:22:09.289546057 -0400
@@ -240,7 +240,7 @@ gfc_post_options (const char **pfilename
     gfc_add_include_path (".", true);

   if (canon_source_file != gfc_source_file)
-    gfc_free ((void *) canon_source_file);
+    gfc_free (CONST_CAST (canon_source_file));

   /* Decide which form the file will be read in as.  */

diff -rup orig/egcc-SVN20070727/gcc/fortran/parse.c egcc-SVN20070727/gcc/fortran/parse.c
--- orig/egcc-SVN20070727/gcc/fortran/parse.c	2007-07-22 23:02:56.000000000 -0400
+++ egcc-SVN20070727/gcc/fortran/parse.c	2007-07-27 12:22:09.293116555 -0400
@@ -2609,7 +2609,7 @@ parse_omp_structured_block (gfc_statemen
 	      && strcmp (cp->ext.omp_name, new_st.ext.omp_name) != 0))
 	gfc_error ("Name after !$omp critical and !$omp end critical does "
 		   "not match at %C");
-      gfc_free ((char *) new_st.ext.omp_name);
+      gfc_free (CONST_CAST (new_st.ext.omp_name));
       break;
     case EXEC_OMP_END_SINGLE:
       cp->ext.omp_clauses->lists[OMP_LIST_COPYPRIVATE]
diff -rup orig/egcc-SVN20070727/gcc/fortran/st.c egcc-SVN20070727/gcc/fortran/st.c
--- orig/egcc-SVN20070727/gcc/fortran/st.c	2007-01-20 20:01:25.000000000 -0500
+++ egcc-SVN20070727/gcc/fortran/st.c	2007-07-27 12:22:09.294050288 -0400
@@ -174,7 +174,7 @@ gfc_free_statement (gfc_code *p)
       break;

     case EXEC_OMP_CRITICAL:
-      gfc_free ((char *) p->ext.omp_name);
+      gfc_free (CONST_CAST (p->ext.omp_name));
       break;

     case EXEC_OMP_FLUSH:
diff -rup orig/egcc-SVN20070727/gcc/gcc.c egcc-SVN20070727/gcc/gcc.c
--- orig/egcc-SVN20070727/gcc/gcc.c	2007-07-26 23:03:38.000000000 -0400
+++ egcc-SVN20070727/gcc/gcc.c	2007-07-27 12:22:09.303780116 -0400
@@ -1876,7 +1876,7 @@ set_spec (const char *name, const char *

   /* Free the old spec.  */
   if (old_spec && sl->alloc_p)
-    free ((void *) old_spec);
+    free (CONST_CAST(old_spec));

   sl->alloc_p = 1;
 }
@@ -2181,7 +2181,7 @@ read_specs (const char *filename, int ma

 	      set_spec (p2, *(sl->ptr_spec));
 	      if (sl->alloc_p)
-		free ((void *) *(sl->ptr_spec));
+		free (CONST_CAST (*(sl->ptr_spec)));

 	      *(sl->ptr_spec) = "";
 	      sl->alloc_p = 0;
@@ -2531,18 +2531,18 @@ for_each_path (const struct path_prefix
 	 Don't repeat any we have already seen.  */
       if (multi_dir)
 	{
-	  free ((char *) multi_dir);
+	  free (CONST_CAST (multi_dir));
 	  multi_dir = NULL;
-	  free ((char *) multi_suffix);
+	  free (CONST_CAST (multi_suffix));
 	  multi_suffix = machine_suffix;
-	  free ((char *) just_multi_suffix);
+	  free (CONST_CAST (just_multi_suffix));
 	  just_multi_suffix = just_machine_suffix;
 	}
       else
 	skip_multi_dir = true;
       if (multi_os_dir)
 	{
-	  free ((char *) multi_os_dir);
+	  free (CONST_CAST (multi_os_dir));
 	  multi_os_dir = NULL;
 	}
       else
@@ -2551,12 +2551,12 @@ for_each_path (const struct path_prefix

   if (multi_dir)
     {
-      free ((char *) multi_dir);
-      free ((char *) multi_suffix);
-      free ((char *) just_multi_suffix);
+      free (CONST_CAST (multi_dir));
+      free (CONST_CAST (multi_suffix));
+      free (CONST_CAST (just_multi_suffix));
     }
   if (multi_os_dir)
-    free ((char *) multi_os_dir);
+    free (CONST_CAST (multi_os_dir));
   if (ret != path)
     free (path);
   return ret;
@@ -2963,7 +2963,7 @@ execute (void)
       errmsg = pex_run (pex,
 			((i + 1 == n_commands ? PEX_LAST : 0)
 			 | (string == commands[i].prog ? PEX_SEARCH : 0)),
-			string, (char * const *) commands[i].argv,
+			string, (char * const *) CONST_CAST (commands[i].argv),
 			NULL, NULL, &err);
       if (errmsg != NULL)
 	{
@@ -2977,7 +2977,7 @@ execute (void)
 	}

       if (string != commands[i].prog)
-	free ((void *) string);
+	free (CONST_CAST (string));
     }

   execution_count++;
@@ -5041,7 +5041,7 @@ do_spec_1 (const char *spec, int inswitc
                   for (i = 0, j = 0; i < max; i++)
                     if (outfiles[i])
                       {
-                        argv[j] = (char *) outfiles[i];
+                        argv[j] = (char *) CONST_CAST (outfiles[i]);
                         j++;
                       }
                   argv[j] = NULL;
@@ -6017,13 +6017,13 @@ give_switch (int switchnum, int omit_fir
 	      while (length-- && !IS_DIR_SEPARATOR (arg[length]))
 		if (arg[length] == '.')
 		  {
-		    ((char *)arg)[length] = 0;
+		    ((char *)CONST_CAST(arg))[length] = 0;
 		    dot = 1;
 		    break;
 		  }
 	      do_spec_1 (arg, 1, NULL);
 	      if (dot)
-		((char *)arg)[length] = '.';
+		((char *)CONST_CAST(arg))[length] = '.';
 	      do_spec_1 (suffix_subst, 1, NULL);
 	    }
 	  else
@@ -7476,7 +7476,7 @@ set_multilib_dir (void)
   if (multilib_dir == NULL && multilib_os_dir != NULL
       && strcmp (multilib_os_dir, ".") == 0)
     {
-      free ((char *) multilib_os_dir);
+      free (CONST_CAST (multilib_os_dir));
       multilib_os_dir = NULL;
     }
   else if (multilib_dir != NULL && multilib_os_dir == NULL)
diff -rup orig/egcc-SVN20070727/gcc/gengtype-parse.c egcc-SVN20070727/gcc/gengtype-parse.c
--- orig/egcc-SVN20070727/gcc/gengtype-parse.c	2007-07-26 23:03:55.000000000 -0400
+++ egcc-SVN20070727/gcc/gengtype-parse.c	2007-07-27 12:22:09.306731752 -0400
@@ -197,9 +197,9 @@ string_seq (void)

       l1 = strlen (s1);
       l2 = strlen (s2);
-      buf = XRESIZEVEC (char, s1, l1 + l2 + 1);
+      buf = XRESIZEVEC (char, CONST_CAST(s1), l1 + l2 + 1);
       memcpy (buf + l1, s2, l2 + 1);
-      XDELETE (s2);
+      XDELETE (CONST_CAST (s2));
       s1 = buf;
     }
   return s1;
@@ -221,8 +221,8 @@ typedef_name (void)
       c2 = require (ID);
       require (')');
       r = concat ("VEC_", c1, "_", c2, (char *)0);
-      free ((void *)c1);
-      free ((void *)c2);
+      free (CONST_CAST (c1));
+      free (CONST_CAST (c2));
       return r;
     }
   else
diff -rup orig/egcc-SVN20070727/gcc/java/jcf-parse.c egcc-SVN20070727/gcc/java/jcf-parse.c
--- orig/egcc-SVN20070727/gcc/java/jcf-parse.c	2007-07-25 15:13:16.000000000 -0400
+++ egcc-SVN20070727/gcc/java/jcf-parse.c	2007-07-27 12:22:09.310355301 -0400
@@ -1302,7 +1302,7 @@ read_class (tree name)
       if (path_name == 0)
 	return 0;
       else
-	free((char *) path_name);
+	free(CONST_CAST (path_name));
     }

   current_jcf = jcf;
@@ -1784,7 +1784,7 @@ java_parse_file (int set_yydebug ATTRIBU
       file_list = list;
     }
   else
-    list = (char *) main_input_filename;
+    list = (char *) CONST_CAST (main_input_filename);

   while (list)
     {
diff -rup orig/egcc-SVN20070727/gcc/java/jcf.h egcc-SVN20070727/gcc/java/jcf.h
--- orig/egcc-SVN20070727/gcc/java/jcf.h	2007-01-13 20:02:03.000000000 -0500
+++ egcc-SVN20070727/gcc/java/jcf.h	2007-07-27 12:22:09.311437309 -0400
@@ -165,8 +165,8 @@ typedef struct JCF GTY(()) {
 #define JCF_FINISH(JCF) { \
   CPOOL_FINISH(&(JCF)->cpool); \
   if ((JCF)->buffer) free ((JCF)->buffer); \
-  if ((JCF)->filename) free ((char *) (JCF)->filename); \
-  if ((JCF)->classname) free ((char *) (JCF)->classname); \
+  if ((JCF)->filename) free (CONST_CAST ((JCF)->filename)); \
+  if ((JCF)->classname) free (CONST_CAST ((JCF)->classname)); \
   (JCF)->finished = 1; }

 #define CPOOL_INIT(CPOOL) \
diff -rup orig/egcc-SVN20070727/gcc/passes.c egcc-SVN20070727/gcc/passes.c
--- orig/egcc-SVN20070727/gcc/passes.c	2007-07-26 23:04:10.000000000 -0400
+++ egcc-SVN20070727/gcc/passes.c	2007-07-27 12:22:09.313279731 -0400
@@ -1152,7 +1152,7 @@ execute_one_pass (struct tree_opt_pass *
   /* Flush and close dump file.  */
   if (dump_file_name)
     {
-      free ((char *) dump_file_name);
+      free (CONST_CAST (dump_file_name));
       dump_file_name = NULL;
     }

diff -rup orig/egcc-SVN20070727/gcc/prefix.c egcc-SVN20070727/gcc/prefix.c
--- orig/egcc-SVN20070727/gcc/prefix.c	2007-07-26 23:04:13.000000000 -0400
+++ egcc-SVN20070727/gcc/prefix.c	2007-07-27 12:22:09.314993723 -0400
@@ -266,7 +266,7 @@ update_path (const char *path, const cha

       result = concat (key, &path[len], NULL);
       if (free_key)
-	free ((char *) key);
+	free (CONST_CAST (key));
       result = translate_name (result);
     }
   else
diff -rup orig/egcc-SVN20070727/gcc/pretty-print.c egcc-SVN20070727/gcc/pretty-print.c
--- orig/egcc-SVN20070727/gcc/pretty-print.c	2007-07-26 23:03:41.000000000 -0400
+++ egcc-SVN20070727/gcc/pretty-print.c	2007-07-27 12:22:09.316952092 -0400
@@ -633,7 +633,7 @@ pp_base_destroy_prefix (pretty_printer *
 {
   if (pp->prefix != NULL)
     {
-      free ((char *) pp->prefix);
+      free (CONST_CAST (pp->prefix));
       pp->prefix = NULL;
     }
 }
diff -rup orig/egcc-SVN20070727/gcc/tree.c egcc-SVN20070727/gcc/tree.c
--- orig/egcc-SVN20070727/gcc/tree.c	2007-07-26 23:03:31.000000000 -0400
+++ egcc-SVN20070727/gcc/tree.c	2007-07-27 12:22:09.326017847 -0400
@@ -1176,8 +1176,8 @@ build_string (int len, const char *str)
   TREE_CONSTANT (s) = 1;
   TREE_INVARIANT (s) = 1;
   TREE_STRING_LENGTH (s) = len;
-  memcpy ((char *) TREE_STRING_POINTER (s), str, len);
-  ((char *) TREE_STRING_POINTER (s))[len] = '\0';
+  memcpy (CONST_CAST (TREE_STRING_POINTER (s)), str, len);
+  ((char *) CONST_CAST (TREE_STRING_POINTER (s)))[len] = '\0';

   return s;
 }

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Richard Guenther]

On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> On Fri, 27 Jul 2007, Richard Guenther wrote:
>
> > On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> > > +union gcc_constcast
> > > +{
> > > +  const void *cv;
> > > +  void *v;
> > > +};
> > > +#define CONST_CAST(X) ((__extension__(union gcc_constcast)(const void *)(X)).v)
> >
> > Can you explain in how this is better than using an explicit cast to the
> > non-const version like in memcpy ((char *)x, ...)?
>
> Sure.  Besides my main point which is silencing -Wcast-qual warnings, the
> CONST_CAST macro is grep-able.  I.e. you can search for the few places
> where it's used and audit if they are correctly applied.  You cannot do
> that with regular C-style casts.  Another reason is that with C-style
> casts there isn't any indication of why it's there.  The CONST_CAST macro
> comments itself to someone reading the code.  (These are some of the
> rationales for the creation of C++'s cast operators.)  The macro isn't
> type-safe like the C++ operator is, but it's a poor man's C option. :-)

Ha!  Maybe a reason to switch to C++ ;)  How about

#ifdef __cplusplus
#define CONST_CAST(x)  .. template magic follows ..
#else
...
#endif

? ;)

Richard.

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Kaveh R. GHAZI]

On Fri, 27 Jul 2007, Richard Guenther wrote:

> On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> > Sure.  Besides my main point which is silencing -Wcast-qual warnings, the
> > CONST_CAST macro is grep-able.  I.e. you can search for the few places
> > where it's used and audit if they are correctly applied.  You cannot do
> > that with regular C-style casts.  Another reason is that with C-style
> > casts there isn't any indication of why it's there.  The CONST_CAST macro
> > comments itself to someone reading the code.  (These are some of the
> > rationales for the creation of C++'s cast operators.)  The macro isn't
> > type-safe like the C++ operator is, but it's a poor man's C option. :-)
>
> Ha!  Maybe a reason to switch to C++ ;)  How about
>
> #ifdef __cplusplus
> #define CONST_CAST(x)  .. template magic follows ..
> #else
> ...
> #endif
>
> ? ;)
> Richard.

I don't think that will work in a generic way.  For static_cast, you need
to supply the stripped type like so:

static_cast<STRIPPED-TYPE>(EXPR)

Since we don't yet have a variant of __typeof to get the qualifer stripped
type for us, we'd need to pass in the stripped type to the macro like so:

#ifdef __cplusplus
#define CONST_CAST(X,TYPE)  static_cast<TYPE>(X)

Note this doesn't increase type safety for most of the cases we'll use it
in because we're just passing the pointer into a situation that takes a
void* like free(foo).  I'm not sure this change is worth it so I'd rather
leave this refinement to the folks who care about compiling gcc with a C++
compiler if they care to take this step.

Also, I don't believe the main sources are compilable with a C++ compiler
at this point anyway.  Only the libiberty/ and gcc/cp/ directories use
-Wc++-compat AFAICT.

So may I install the latest version of my patch?

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Gabriel Dos Reis]

"Kaveh R. GHAZI" <ghazi@caip.rutgers.edu> writes:

| On Fri, 27 Jul 2007, Richard Guenther wrote:
| 
| > On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
| > > Sure.  Besides my main point which is silencing -Wcast-qual warnings, the
| > > CONST_CAST macro is grep-able.  I.e. you can search for the few places
| > > where it's used and audit if they are correctly applied.  You cannot do
| > > that with regular C-style casts.  Another reason is that with C-style
| > > casts there isn't any indication of why it's there.  The CONST_CAST macro
| > > comments itself to someone reading the code.  (These are some of the
| > > rationales for the creation of C++'s cast operators.)  The macro isn't
| > > type-safe like the C++ operator is, but it's a poor man's C option. :-)
| >
| > Ha!  Maybe a reason to switch to C++ ;)  How about

Indeed :-)

[...]

| Also, I don't believe the main sources are compilable with a C++ compiler
| at this point anyway.  Only the libiberty/ and gcc/cp/ directories use
| -Wc++-compat AFAICT.
| 
| So may I install the latest version of my patch?

OK for the diagnostics part (not the system.h).

-- Gaby

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Kaveh R. GHAZI]

On Fri, 27 Jul 2007, Kaveh R. GHAZI wrote:

> On Fri, 27 Jul 2007, Richard Guenther wrote:
>
> > On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> > > Sure.  Besides my main point which is silencing -Wcast-qual warnings, the
> > > CONST_CAST macro is grep-able.  I.e. you can search for the few places
> > > where it's used and audit if they are correctly applied.  You cannot do
> > > that with regular C-style casts.  Another reason is that with C-style
> > > casts there isn't any indication of why it's there.  The CONST_CAST macro
> > > comments itself to someone reading the code.  (These are some of the
> > > rationales for the creation of C++'s cast operators.)  The macro isn't
> > > type-safe like the C++ operator is, but it's a poor man's C option. :-)
> >
> > Ha!  Maybe a reason to switch to C++ ;)  How about
> >
> > #ifdef __cplusplus
> > #define CONST_CAST(x)  .. template magic follows ..
> > #else
> > ...
> > #endif
> >
> > ? ;)
> > Richard.
>
> I don't think that will work in a generic way.  For static_cast, you need
> to supply the stripped type like so:
>
> static_cast<STRIPPED-TYPE>(EXPR)
>
> Since we don't yet have a variant of __typeof to get the qualifer stripped
> type for us, we'd need to pass in the stripped type to the macro like so:
>
> #ifdef __cplusplus
> #define CONST_CAST(X,TYPE)  static_cast<TYPE>(X)
>
> Note this doesn't increase type safety for most of the cases we'll use it
> in because we're just passing the pointer into a situation that takes a
> void* like free(foo).  I'm not sure this change is worth it so I'd rather
> leave this refinement to the folks who care about compiling gcc with a C++
> compiler if they care to take this step.
>
> Also, I don't believe the main sources are compilable with a C++ compiler
> at this point anyway.  Only the libiberty/ and gcc/cp/ directories use
> -Wc++-compat AFAICT.
>
> So may I install the latest version of my patch?


Richard, do you have any remaining objections to this patch?
http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html

If there is some C++ magic you want, can you please suggest something?
I.e. do you want me to add a TYPE parameter to CONST_CAST?  Perhaps
CONST_CAST_WITH_TYPE?  Otherwise I'd like to install it.

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Gabriel Dos Reis]

"Kaveh R. GHAZI" <ghazi@caip.rutgers.edu> writes:

| On Fri, 27 Jul 2007, Kaveh R. GHAZI wrote:
| 
| > On Fri, 27 Jul 2007, Richard Guenther wrote:
| >
| > > On 7/27/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
| > > > Sure.  Besides my main point which is silencing -Wcast-qual warnings, the
| > > > CONST_CAST macro is grep-able.  I.e. you can search for the few places
| > > > where it's used and audit if they are correctly applied.  You cannot do
| > > > that with regular C-style casts.  Another reason is that with C-style
| > > > casts there isn't any indication of why it's there.  The CONST_CAST macro
| > > > comments itself to someone reading the code.  (These are some of the
| > > > rationales for the creation of C++'s cast operators.)  The macro isn't
| > > > type-safe like the C++ operator is, but it's a poor man's C option. :-)
| > >
| > > Ha!  Maybe a reason to switch to C++ ;)  How about
| > >
| > > #ifdef __cplusplus
| > > #define CONST_CAST(x)  .. template magic follows ..
| > > #else
| > > ...
| > > #endif
| > >
| > > ? ;)
| > > Richard.
| >
| > I don't think that will work in a generic way.  For static_cast, you need
| > to supply the stripped type like so:
| >
| > static_cast<STRIPPED-TYPE>(EXPR)
| >
| > Since we don't yet have a variant of __typeof to get the qualifer stripped
| > type for us, we'd need to pass in the stripped type to the macro like so:
| >
| > #ifdef __cplusplus
| > #define CONST_CAST(X,TYPE)  static_cast<TYPE>(X)
| >
| > Note this doesn't increase type safety for most of the cases we'll use it
| > in because we're just passing the pointer into a situation that takes a
| > void* like free(foo).  I'm not sure this change is worth it so I'd rather
| > leave this refinement to the folks who care about compiling gcc with a C++
| > compiler if they care to take this step.
| >
| > Also, I don't believe the main sources are compilable with a C++ compiler
| > at this point anyway.  Only the libiberty/ and gcc/cp/ directories use
| > -Wc++-compat AFAICT.
| >
| > So may I install the latest version of my patch?
| 
| 
| Richard, do you have any remaining objections to this patch?
| http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html
| 
| If there is some C++ magic you want, can you please suggest something?

Since this patch does not prepare for enabling C++ codes, I'm fine
with it as it is.  I would not suggest more __typeof magic at this
moment.  Of course, it would have been better if the codes were
written in such a way that the casts were almost never needed.

Richard G.  How strongly do you feel about this?

-- Gaby

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Mark Mitchell]

Gabriel Dos Reis wrote:

> | Richard, do you have any remaining objections to this patch?
> | http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html

I'm not very keen on the use of a union here, just as a way to outwit
our own warning machinery.  For grep-ability, just using CONST_CAST with
a definition that is a traditional C-style cast, would work fine.  Or,
can we go through an inline function, with warnings disabled on the
inline function?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Gabriel Dos Reis]

On Sun, 5 Aug 2007, Mark Mitchell wrote:

| Gabriel Dos Reis wrote:
| 
| > | Richard, do you have any remaining objections to this patch?
| > | http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html
| 
| I'm not very keen on the use of a union here, just as a way to outwit
| our own warning machinery.

A good point that the union is overkill.

|  For grep-ability, just using CONST_CAST with
| a definition that is a traditional C-style cast, would work fine. 

The compiler will "see" through the macro expansion and will start
warning when -Wcast-qual is on.

| Or,
| can we go through an inline function, with warnings disabled on the
| inline function?

I don't know whether this works; Kaveh can you try it?
(You would still have to write the function name all caps).

-- Gaby

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Kaveh R. GHAZI]

On Sun, 5 Aug 2007, Gabriel Dos Reis wrote:

> On Sun, 5 Aug 2007, Mark Mitchell wrote:
>
> | Gabriel Dos Reis wrote:
> |
> | > | Richard, do you have any remaining objections to this patch?
> | > | http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html
> |
> | I'm not very keen on the use of a union here, just as a way to outwit
> | our own warning machinery.
>
> A good point that the union is overkill.

I used a union because it's the only solution I found that works.  I would
be happy to use something else more elegant if someone suggests something
concrete.


> |  For grep-ability, just using CONST_CAST with
> | a definition that is a traditional C-style cast, would work fine.
>
> The compiler will "see" through the macro expansion and will start
> warning when -Wcast-qual is on.

Right.



> | Or,
> | can we go through an inline function, with warnings disabled on the
> | inline function?


How do we disable warnings on the fly inside an inline function?  Did
someone implement "#pragma nowarn foo" when I wasn't looking?



> I don't know whether this works; Kaveh can you try it?
> (You would still have to write the function name all caps).
> -- Gaby

Sure, but try what exactly?

I attempted various inline function permutations and the only thing that
works is putting a union inside the inline function.  (By "works" I mean
silences -Wcast-qual warning.)  I don't see how putting the union inside
an inline function as any better than using a macro, in fact the inline
function version is slightly worse.  It can't use __typeof() like Richard
suggested because the function prototype hides the arguments actual type.

In light of this, I'd like permission to install my existing patch.  If
someone can suggest a better definition for CONST_CAST() and/or write an
inline function to replace it, it's merely a one-line change to system.h
as a follow-up.  Until then, the union hack is the only thing I can think
of that actually does the job.

Okay?

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Mark Mitchell]

Kaveh R. GHAZI wrote:

> In light of this, I'd like permission to install my existing patch.  If
> someone can suggest a better definition for CONST_CAST() and/or write an
> inline function to replace it, it's merely a one-line change to system.h
> as a follow-up.  Until then, the union hack is the only thing I can think
> of that actually does the job.

I think it's pretty ugly.  Is this what we want our users to do when
they have the same issue?

Until we provide some way to turn off warnings in a local region of
code, I'd rather we just accept that we can't be quite -Wcast-qual
clean.  Use it as an incentive to fix the compiler, either to make the
warning smarter (by, for example, allowing casts in the argument to
"free", or by allowing fine-grained warning control).

I'm not going to reject the patch, but I'm not going to approve it
either.  Color me abstaining. :-)

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Kaveh R. GHAZI]

On Sun, 5 Aug 2007, Mark Mitchell wrote:

> Until we provide some way to turn off warnings in a local region of
> code, I'd rather we just accept that we can't be quite -Wcast-qual
> clean.  Use it as an incentive to fix the compiler, either to make the
> warning smarter (by, for example, allowing casts in the argument to
> "free", or by allowing fine-grained warning control).

How about a __nowarn__ keyword that works similar to __extension__?
Instead of twiddling the pedantic flag, it would push/pop the
inhibit_warnings variable inside gcc to silence warnings at the statement
level.

I'm about halfway through a patch.  If you're amenable to the idea I'll
finish it up and post it.

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Gabriel Dos Reis]

On Mon, 6 Aug 2007, Kaveh R. GHAZI wrote:

| On Sun, 5 Aug 2007, Mark Mitchell wrote:
| 
| > Until we provide some way to turn off warnings in a local region of
| > code, I'd rather we just accept that we can't be quite -Wcast-qual
| > clean.  Use it as an incentive to fix the compiler, either to make the
| > warning smarter (by, for example, allowing casts in the argument to
| > "free", or by allowing fine-grained warning control).
| 
| How about a __nowarn__ keyword that works similar to __extension__?
| Instead of twiddling the pedantic flag, it would push/pop the
| inhibit_warnings variable inside gcc to silence warnings at the statement
| level.
| 
| I'm about halfway through a patch.  If you're amenable to the idea I'll
| finish it up and post it.

I would have thought that DJ's idea of using #pragma in code text
to turn on/off warning coupled with an inline function would do.

-- Gaby

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Kaveh R. GHAZI]

On Mon, 6 Aug 2007, Gabriel Dos Reis wrote:

> On Mon, 6 Aug 2007, Kaveh R. GHAZI wrote:
>
> | How about a __nowarn__ keyword that works similar to __extension__?
> | Instead of twiddling the pedantic flag, it would push/pop the
> | inhibit_warnings variable inside gcc to silence warnings at the statement
> | level.
> |
> | I'm about halfway through a patch.  If you're amenable to the idea I'll
> | finish it up and post it.
>
> I would have thought that DJ's idea of using #pragma in code text
> to turn on/off warning coupled with an inline function would do.
> -- Gaby

It would do, if it was finished.  I don't know where that stands.
Perhaps DJ could give us an update.

BTW, I'm not sure I ever saw a specific description of how the #pragma
syntax would look like.  IMHO, I wouldn't do an on/off interface.
Instead I think push/pop makes more sense.  That way you restore the
setting specified on the command line after the code region you're
interested in. E.g. the __extension__ keyword uses push/pop semantics so
that the -pedantic flag is left unchanged after the region it affects.

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[DJ Delorie]

> It would do, if it was finished.  I don't know where that stands.
> Perhaps DJ could give us an update.

It got bogged down in arguments about how expression-specific warning
settings would work, and inlined functions with different warning
sets.  Then it got into statement-specific warning changesets and
deltas.  We never got around to implementing any of the compromises.

Currently, you can wrap a function with the pragmas and sometimes it
works the way you want, especially if you disable whole-file
optimizations and tree inlining ;-)

Thus, we only allow (I don't know if it's enforced) the pragmas before
the first function definition.

I think technically we can keep track of the changes efficiently, if
we have "change point" data structures that have the warning number,
new action, and a pointer to the previous change point.  That's 12
bytes per #pragma, with a little more work when you actually print a
message (to follow the chain back to find the most recent change for
that warning, or to the beginning, where we use the master table).

That also allows us to push/pop by just starting a new chain at some
previous change point.  Scoping would do the same thing.  No need for
forward pointers, just the back pointers.

The hard part is mapping a "where" in the code with a "where" in the
change point chain.

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Gabriel Dos Reis]

DJ Delorie <dj@redhat.com> writes:

| > It would do, if it was finished.  I don't know where that stands.
| > Perhaps DJ could give us an update.
| 
| It got bogged down in arguments about how expression-specific warning
| settings would work, and inlined functions with different warning
| sets.  Then it got into statement-specific warning changesets and
| deltas.  We never got around to implementing any of the compromises.
| 
| Currently, you can wrap a function with the pragmas and sometimes it
| works the way you want, especially if you disable whole-file
| optimizations and tree inlining ;-)
| 
| Thus, we only allow (I don't know if it's enforced) the pragmas before
| the first function definition.

If the current implementation works at function definition level, then
I believe Kaveh has all he needs.

-- Gaby

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Gabriel Dos Reis]

On Mon, 6 Aug 2007, Kaveh R. GHAZI wrote:

| On Mon, 6 Aug 2007, Gabriel Dos Reis wrote:
| 
| > On Mon, 6 Aug 2007, Kaveh R. GHAZI wrote:
| >
| > | How about a __nowarn__ keyword that works similar to __extension__?
| > | Instead of twiddling the pedantic flag, it would push/pop the
| > | inhibit_warnings variable inside gcc to silence warnings at the statement
| > | level.
| > |
| > | I'm about halfway through a patch.  If you're amenable to the idea I'll
| > | finish it up and post it.
| >
| > I would have thought that DJ's idea of using #pragma in code text
| > to turn on/off warning coupled with an inline function would do.
| > -- Gaby
| 
| It would do, if it was finished.  I don't know where that stands.
| Perhaps DJ could give us an update.
| 
| BTW, I'm not sure I ever saw a specific description of how the #pragma
| syntax would look like.  IMHO, I wouldn't do an on/off interface.
| Instead I think push/pop makes more sense.

Yes, you're right.  In fact, I believe push/pop was the model that DJ
advocated.  DJ?

-- Gaby

Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro

[Mark Mitchell]

Kaveh R. GHAZI wrote:

> How about a __nowarn__ keyword that works similar to __extension__?
> Instead of twiddling the pedantic flag, it would push/pop the
> inhibit_warnings variable inside gcc to silence warnings at the statement
> level.
> 
> I'm about halfway through a patch.  If you're amenable to the idea I'll
> finish it up and post it.

I don't have any objections to that.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Add a __nowarn__ keyword

[Kaveh R. GHAZI]

As discussed here:
http://gcc.gnu.org/ml/gcc-patches/2007-08/msg00345.html

this patch adds a __nowarn__ keyword to the C-family languages.  It is
analogous to __extension__ except that instead of eliding -pedantic and
-Wtraditional warnings, the __nowarn__ keyword flips the -w/--no-warnings
flag (internally in GCC the "inhibit_warnings" variable) on a
per-statement basis.  Like __extension__, the __nowarn__ keyword uses a
push/pop mechanism to restore the warning state after it's used to that
originally specified on the command line.

If this is acceptable, I'll redo my CONST_CAST patch to use __nowarn__
like so:

#if GCC_VERSION >= 4003
use __nowarn__ in CONST_CAST
#elif __GNUC__
use union in CONST_CAST
#else
do a regular cast in CONST_CAST
#endif

Although the union hack was considered ugly, I still think we should use
it to silence the warnings for older GCC versions.  However as Mark
requested, we now have a more elegant, sanctioned, official way to
recommend to users on how to silence these kinds of warnings.

I believe a future refinement could be to allow an argument to __nowarn__
which specifies a specific warning to silence (rather than all of themn).
This might solve some of the problems DJ described in the #pragma warning
controls.  Perhaps a project for down the road. For now, __nowarn__
without arguments does the job, albeit bluntly.

Patch below tested in conjunction with my updated CONST_CAST patch on
sparc-sun-solaris2.10, no regressions.  I added -Wcast-qual to the
bootstrap and ensured that __nowarn__ does what it's supposed to, namely
silence warnings.

Patch below okay for mainline?

Is the CONST_CAST patch okay now too?

		Thanks,
		--Kaveh


2007-08-07  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

	* c-common.h (enum rid): Add RID_NOWARN.
	* c-parser.c (reswords): Add "__nowarn__".
	(disable_warning_diagnostics, restore_warning_diagnostics): New.
	(c_parser_external_declaration, c_parser_struct_declaration,
	c_parser_compound_statement_nostart, c_parser_for_statement,
	c_parser_unary_expression): Handle RID_NOWARN.
	* doc/extend.texi: Document __nowarn__.
	* doc/invoke.texi: Likewise.

cp:
	lex.c (reswords): Add "__nowarn__".
	parser.c (cp_parser_nowarn_opt): New.
	(cp_parser_unary_expression, cp_parser_declaration,
	cp_parser_block_declaration, cp_parser_member_declaration): Handle
	RID_NOWARN.

diff -rup orig/egcc-SVN20070804/gcc/c-common.h egcc-SVN20070804/gcc/c-common.h
--- orig/egcc-SVN20070804/gcc/c-common.h	2007-07-27 13:50:51.000000000 -0400
+++ egcc-SVN20070804/gcc/c-common.h	2007-08-06 09:12:05.320743836 -0400
@@ -70,7 +70,7 @@ enum rid
   /* C extensions */
   RID_ASM,       RID_TYPEOF,   RID_ALIGNOF,  RID_ATTRIBUTE,  RID_VA_ARG,
   RID_EXTENSION, RID_IMAGPART, RID_REALPART, RID_LABEL,      RID_CHOOSE_EXPR,
-  RID_TYPES_COMPATIBLE_P,
+  RID_TYPES_COMPATIBLE_P,      RID_NOWARN,
   RID_DFLOAT32, RID_DFLOAT64, RID_DFLOAT128,

   /* Too many ways of getting the name of a function as a string */
diff -rup orig/egcc-SVN20070804/gcc/c-parser.c egcc-SVN20070804/gcc/c-parser.c
--- orig/egcc-SVN20070804/gcc/c-parser.c	2007-07-26 23:04:11.000000000 -0400
+++ egcc-SVN20070804/gcc/c-parser.c	2007-08-07 00:18:50.499508794 -0400
@@ -103,6 +103,7 @@ static const struct resword reswords[] =
   { "__inline",		RID_INLINE,	0 },
   { "__inline__",	RID_INLINE,	0 },
   { "__label__",	RID_LABEL,	0 },
+  { "__nowarn__",	RID_NOWARN,	0 },
   { "__real",		RID_REALPART,	0 },
   { "__real__",		RID_REALPART,	0 },
   { "__restrict",	RID_RESTRICT,	0 },
@@ -929,6 +930,25 @@ restore_extension_diagnostics (int flags
   flag_iso = (flags >> 3) & 1;
 }

+/* Save the warning flags which are controlled by __nowarn__.  */
+
+static inline int
+disable_warning_diagnostics (void)
+{
+  int ret = inhibit_warnings;
+  inhibit_warnings = 1;
+  return ret;
+}
+
+/* Restore the warning flags which are controlled by __nowarn__.
+   FLAGS is the return value from disable_warning_diagnostics.  */
+
+static inline void
+restore_warning_diagnostics (int flags)
+{
+  inhibit_warnings = flags;
+}
+
 /* Possibly kinds of declarator to parse.  */
 typedef enum c_dtr_syn {
   /* A normal declarator with an identifier.  */
@@ -1108,6 +1128,12 @@ c_parser_external_declaration (c_parser
 	  c_parser_external_declaration (parser);
 	  restore_extension_diagnostics (ext);
 	  break;
+	case RID_NOWARN:
+	  ext = disable_warning_diagnostics ();
+	  c_parser_consume_token (parser);
+	  c_parser_external_declaration (parser);
+	  restore_warning_diagnostics (ext);
+	  break;
 	case RID_ASM:
 	  c_parser_asm_definition (parser);
 	  break;
@@ -1987,6 +2013,16 @@ c_parser_struct_declaration (c_parser *p
       restore_extension_diagnostics (ext);
       return decl;
     }
+  if (c_parser_next_token_is_keyword (parser, RID_NOWARN))
+    {
+      int ext;
+      tree decl;
+      ext = disable_warning_diagnostics ();
+      c_parser_consume_token (parser);
+      decl = c_parser_struct_declaration (parser);
+      restore_warning_diagnostics (ext);
+      return decl;
+    }
   specs = build_null_declspecs ();
   c_parser_declspecs (parser, specs, false, true, true);
   if (parser->error)
@@ -3453,6 +3489,37 @@ c_parser_compound_statement_nostart (c_p
 	  else
 	    goto statement;
 	}
+      else if (!last_label
+	       && c_parser_next_token_is_keyword (parser, RID_NOWARN))
+	{
+	  /* __nowarn__ can start a declaration, but is also an
+	     unary operator that can start an expression.  Consume all
+	     but the last of a possible series of __nowarn__ to
+	     determine which.  */
+	  while (c_parser_peek_2nd_token (parser)->type == CPP_KEYWORD
+		 && (c_parser_peek_2nd_token (parser)->keyword
+		     == RID_NOWARN))
+	    c_parser_consume_token (parser);
+	  if (c_token_starts_declspecs (c_parser_peek_2nd_token (parser)))
+	    {
+	      int ext;
+	      ext = disable_warning_diagnostics ();
+	      c_parser_consume_token (parser);
+	      last_label = false;
+	      c_parser_declaration_or_fndef (parser, true, true, true, true);
+	      /* Following the old parser, __nowarn__ does not
+		 disable this diagnostic.  */
+	      restore_warning_diagnostics (ext);
+	      if (last_stmt
+		  && ((pedantic && !flag_isoc99)
+		      || warn_declaration_after_statement))
+		pedwarn_c90 ("%HISO C90 forbids mixed declarations and code",
+			     &loc);
+	      last_stmt = false;
+	    }
+	  else
+	    goto statement;
+	}
       else if (c_parser_next_token_is (parser, CPP_PRAGMA))
 	{
 	  /* External pragmas, and some omp pragmas, are not associated
@@ -4031,6 +4098,28 @@ c_parser_for_statement (c_parser *parser
 	  else
 	    goto init_expr;
 	}
+      else if (c_parser_next_token_is_keyword (parser, RID_NOWARN))
+	{
+	  /* __nowarn__ can start a declaration, but is also an
+	     unary operator that can start an expression.  Consume all
+	     but the last of a possible series of __nowarn__ to
+	     determine which.  */
+	  while (c_parser_peek_2nd_token (parser)->type == CPP_KEYWORD
+		 && (c_parser_peek_2nd_token (parser)->keyword
+		     == RID_NOWARN))
+	    c_parser_consume_token (parser);
+	  if (c_token_starts_declspecs (c_parser_peek_2nd_token (parser)))
+	    {
+	      int ext;
+	      ext = disable_warning_diagnostics ();
+	      c_parser_consume_token (parser);
+	      c_parser_declaration_or_fndef (parser, true, true, true, true);
+	      restore_warning_diagnostics (ext);
+	      check_for_loop_decls ();
+	    }
+	  else
+	    goto init_expr;
+	}
       else
 	{
 	init_expr:
@@ -4824,6 +4913,12 @@ c_parser_unary_expression (c_parser *par
 	  ret = c_parser_cast_expression (parser, NULL);
 	  restore_extension_diagnostics (ext);
 	  return ret;
+	case RID_NOWARN:
+	  c_parser_consume_token (parser);
+	  ext = disable_warning_diagnostics ();
+	  ret = c_parser_cast_expression (parser, NULL);
+	  restore_warning_diagnostics (ext);
+	  return ret;
 	case RID_REALPART:
 	  c_parser_consume_token (parser);
 	  op = c_parser_cast_expression (parser, NULL);
diff -rup orig/egcc-SVN20070804/gcc/cp/lex.c egcc-SVN20070804/gcc/cp/lex.c
--- orig/egcc-SVN20070804/gcc/cp/lex.c	2007-08-02 10:19:35.000000000 -0400
+++ egcc-SVN20070804/gcc/cp/lex.c	2007-08-06 09:11:21.620481305 -0400
@@ -222,6 +222,7 @@ static const struct resword reswords[] =
   { "__inline",		RID_INLINE,	0 },
   { "__inline__",	RID_INLINE,	0 },
   { "__label__",	RID_LABEL,	0 },
+  { "__nowarn__",	RID_NOWARN,	0 },
   { "__null",		RID_NULL,	0 },
   { "__real",		RID_REALPART,	0 },
   { "__real__",		RID_REALPART,	0 },
diff -rup orig/egcc-SVN20070804/gcc/cp/parser.c egcc-SVN20070804/gcc/cp/parser.c
--- orig/egcc-SVN20070804/gcc/cp/parser.c	2007-08-02 22:59:11.000000000 -0400
+++ egcc-SVN20070804/gcc/cp/parser.c	2007-08-06 09:50:18.411866342 -0400
@@ -1853,6 +1853,8 @@ static tree cp_parser_attribute_list
   (cp_parser *);
 static bool cp_parser_extension_opt
   (cp_parser *, int *);
+static bool cp_parser_nowarn_opt
+  (cp_parser *, int *);
 static void cp_parser_label_declaration
   (cp_parser *);

@@ -5244,6 +5246,22 @@ cp_parser_unary_expression (cp_parser *p
 	    return expr;
 	  }

+	case RID_NOWARN:
+	  {
+	    /* The saved value of the --no-warnings flag.  */
+	    int saved_inhibit_warnings;
+	    tree expr;
+
+	    /* Save away the INHIBIT_WARNINGS flag.  */
+	    cp_parser_nowarn_opt (parser, &saved_inhibit_warnings);
+	    /* Parse the cast-expression.  */
+	    expr = cp_parser_simple_cast_expression (parser);
+	    /* Restore the INHIBIT_WARNINGS flag.  */
+	    inhibit_warnings = saved_inhibit_warnings;
+
+	    return expr;
+	  }
+
 	case RID_REALPART:
 	case RID_IMAGPART:
 	  {
@@ -7605,7 +7623,7 @@ cp_parser_declaration (cp_parser* parser
 {
   cp_token token1;
   cp_token token2;
-  int saved_pedantic;
+  int saved_pedantic, saved_inhibit_warnings;
   void *p;

   /* Check for the `__extension__' keyword.  */
@@ -7619,6 +7637,17 @@ cp_parser_declaration (cp_parser* parser
       return;
     }

+  /* Check for the `__nowarn__' keyword.  */
+  if (cp_parser_nowarn_opt (parser, &saved_inhibit_warnings))
+    {
+      /* Parse the qualified declaration.  */
+      cp_parser_declaration (parser);
+      /* Restore the INHIBIT_WARNINGS flag.  */
+      inhibit_warnings = saved_inhibit_warnings;
+
+      return;
+    }
+
   /* Try to figure out what kind of declaration is present.  */
   token1 = *cp_lexer_peek_token (parser->lexer);

@@ -7719,7 +7748,7 @@ cp_parser_block_declaration (cp_parser *
 			     bool      statement_p)
 {
   cp_token *token1;
-  int saved_pedantic;
+  int saved_pedantic, saved_inhibit_warnings;

   /* Check for the `__extension__' keyword.  */
   if (cp_parser_extension_opt (parser, &saved_pedantic))
@@ -7732,6 +7761,17 @@ cp_parser_block_declaration (cp_parser *
       return;
     }

+  /* Check for the `__nowarn__' keyword.  */
+  if (cp_parser_nowarn_opt (parser, &saved_inhibit_warnings))
+    {
+      /* Parse the qualified declaration.  */
+      cp_parser_block_declaration (parser, statement_p);
+      /* Restore the INHIBIT_WARNINGS flag.  */
+      inhibit_warnings = saved_inhibit_warnings;
+
+      return;
+    }
+
   /* Peek at the next token to figure out which kind of declaration is
      present.  */
   token1 = cp_lexer_peek_token (parser->lexer);
@@ -14690,7 +14730,7 @@ cp_parser_member_declaration (cp_parser*
   int declares_class_or_enum;
   bool friend_p;
   cp_token *token;
-  int saved_pedantic;
+  int saved_pedantic, saved_inhibit_warnings;

   /* Check for the `__extension__' keyword.  */
   if (cp_parser_extension_opt (parser, &saved_pedantic))
@@ -14703,6 +14743,17 @@ cp_parser_member_declaration (cp_parser*
       return;
     }

+  /* Check for the `__nowarn__' keyword.  */
+  if (cp_parser_nowarn_opt (parser, &saved_inhibit_warnings))
+    {
+      /* Recurse.  */
+      cp_parser_member_declaration (parser);
+      /* Restore the old value of the INHIBIT_WARNINGS flag.  */
+      inhibit_warnings = saved_inhibit_warnings;
+
+      return;
+    }
+
   /* Check for a template-declaration.  */
   if (cp_lexer_next_token_is_keyword (parser->lexer, RID_TEMPLATE))
     {
@@ -15909,6 +15960,32 @@ cp_parser_extension_opt (cp_parser* pars
   return false;
 }

+/* Parse an optional `__nowarn__' keyword.  Returns TRUE if it is
+   present, and FALSE otherwise.  *SAVED_INHIBIT_WARNINGS is set to
+   the current value of the INHIBIT_WARNINGS flag, regardless of
+   whether or not the `__nowarn__' keyword is present.  The caller is
+   responsible for restoring the value of the INHIBIT_WARNINGS flag.  */
+
+static bool
+cp_parser_nowarn_opt (cp_parser* parser, int* saved_inhibit_warnings)
+{
+  /* Save the old value of the inhibit_warnings flag.  */
+  *saved_inhibit_warnings = inhibit_warnings;
+
+  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_NOWARN))
+    {
+      /* Consume the `__nowarn__' token.  */
+      cp_lexer_consume_token (parser->lexer);
+      /* We're not issuing warnings while the `__nowarn__' keyword is
+	 in effect.  */
+      inhibit_warnings = 1;
+
+      return true;
+    }
+
+  return false;
+}
+
 /* Parse a label declaration.

    label-declaration:
diff -rup orig/egcc-SVN20070804/gcc/doc/extend.texi egcc-SVN20070804/gcc/doc/extend.texi
--- orig/egcc-SVN20070804/gcc/doc/extend.texi	2007-07-25 13:08:49.000000000 -0400
+++ egcc-SVN20070804/gcc/doc/extend.texi	2007-08-07 00:30:34.373054367 -0400
@@ -4910,6 +4910,13 @@ prevent such warnings within one express
 @code{__extension__} before the expression.  @code{__extension__} has no
 effect aside from this.

+@findex __nowarn__
+Sometimes warnings are unavoidable and can make using @option{-Werror}
+in combination with warning flags difficult or impossible.  As a last
+resort, you can apply the @code{__nowarn__} keyword before such
+expressions to silence all warnings from only that location.
+@code{__nowarn__} has no effect aside from this.
+
 @node Incomplete Enums
 @section Incomplete @code{enum} Types

diff -rup orig/egcc-SVN20070804/gcc/doc/invoke.texi egcc-SVN20070804/gcc/doc/invoke.texi
--- orig/egcc-SVN20070804/gcc/doc/invoke.texi	2007-08-01 23:03:31.000000000 -0400
+++ egcc-SVN20070804/gcc/doc/invoke.texi	2007-08-07 00:33:05.379095445 -0400
@@ -1295,7 +1295,7 @@ it disables recognition of C++ style @sa
 the @code{inline} keyword.

 The alternate keywords @code{__asm__}, @code{__extension__},
-@code{__inline__} and @code{__typeof__} continue to work despite
+@code{__inline__}, @code{__nowarn__} and @code{__typeof__} continue to work despite
 @option{-ansi}.  You would not want to use them in an ISO C program, of
 course, but it is useful to put them in header files that might be included
 in compilations done with @option{-ansi}.  Alternate predefined macros

Re: Add a __nowarn__ keyword

[Manuel López-Ibáñez]

I am not sure whether this is a good idea since we already have the
#pragma GCC diagnostics. But yes, the pragma does not work (yet) at
the statement level. Also, do any other compilers have a similar
thing? What keyword do they use? Perhaps, we could use it internally
for GCC 4.3 in order to test it but don't expose it as a new feature
until GCC 4.4 is in stage1 to allow users to play with it.

About the patch itself, I don't understand why you use one approach
for the C parser and a different one for the C++ parser. I think it
would be advantageous to use the same approach even if you cannot
share the relevant functions. I prefer the  cp_parser_nowarn_opt
approach: the resulting code looks simpler.

Also, the patch needs testcases. And a follow-up for GCC 4.3 webpage
if we want to expose this to users.

Cheers,

Manuel.

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 347 bytes --]

On Wed, 8 Aug 2007, Manuel López-Ibáñez wrote:

| I am not sure whether this is a good idea since we already have the
| #pragma GCC diagnostics. But yes, the pragma does not work (yet) at
| the statement level.

If I understand DJ correctly, they worked at the function definition level;
that is all we need.  Kaveh, did that not work?

-- Gaby

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Wed, 8 Aug 2007, Gabriel Dos Reis wrote:

> | I am not sure whether this is a good idea since we already have the
> | #pragma GCC diagnostics. But yes, the pragma does not work (yet) at
> | the statement level.
>
> If I understand DJ correctly, they worked at the function definition level;
> that is all we need.  Kaveh, did that not work?
> -- Gaby

I tried using the #pragma on an inline CONST_CAST function, it did not
work for -Wcast-qual warnings.  I didn't bother trying to see if it worked
for any others.

#pragma GCC diagnostic ignored "-Wcast-qual"
extern __inline void *CONST_CAST(const void *cv)
{
  return (void *)cv;
}

% gcc foo.c -Wcast-qual -c
nw.c: In function 'CONST_CAST':
nw.c:4: warning: cast discards qualifiers from pointer target type

So this is not usable for my purposes.

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

On Wed, 8 Aug 2007, Kaveh R. GHAZI wrote:

| On Wed, 8 Aug 2007, Gabriel Dos Reis wrote:
| 
| > | I am not sure whether this is a good idea since we already have the
| > | #pragma GCC diagnostics. But yes, the pragma does not work (yet) at
| > | the statement level.
| >
| > If I understand DJ correctly, they worked at the function definition level;
| > that is all we need.  Kaveh, did that not work?
| > -- Gaby
| 
| I tried using the #pragma on an inline CONST_CAST function, it did not
| work for -Wcast-qual warnings.  I didn't bother trying to see if it worked
| for any others.
| 
| #pragma GCC diagnostic ignored "-Wcast-qual"
| extern __inline void *CONST_CAST(const void *cv)
| {
|   return (void *)cv;
| }
| 
| % gcc foo.c -Wcast-qual -c
| nw.c: In function 'CONST_CAST':
| nw.c:4: warning: cast discards qualifiers from pointer target type

I believe you have discovered a bug in the compiler :-)

-- Gaby

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Wed, 8 Aug 2007, Gabriel Dos Reis wrote:

> On Wed, 8 Aug 2007, Kaveh R. GHAZI wrote:
>
> | On Wed, 8 Aug 2007, Gabriel Dos Reis wrote:
> |
> | > | I am not sure whether this is a good idea since we already have the
> | > | #pragma GCC diagnostics. But yes, the pragma does not work (yet) at
> | > | the statement level.
> | >
> | > If I understand DJ correctly, they worked at the function definition level;
> | > that is all we need.  Kaveh, did that not work?
> | > -- Gaby
> |
> | I tried using the #pragma on an inline CONST_CAST function, it did not
> | work for -Wcast-qual warnings.  I didn't bother trying to see if it worked
> | for any others.
> |
> | #pragma GCC diagnostic ignored "-Wcast-qual"
> | extern __inline void *CONST_CAST(const void *cv)
> | {
> |   return (void *)cv;
> | }
> |
> | % gcc foo.c -Wcast-qual -c
> | nw.c: In function 'CONST_CAST':
> | nw.c:4: warning: cast discards qualifiers from pointer target type
>
> I believe you have discovered a bug in the compiler :-)
> -- Gaby

Perhaps.  The documentation says not all warnings are modifiable.  I
didn't check whether -Wcast-qual is one of them or if this is in fact a
bug.  IMHO because of the problems DJ noted in his summary, I believe
using a #pragma interface is fatally flawed.  In addition to what he said,
you also can't use #pragma in a macro definition.

In contrast, the keyword mechanism as shown by the __extension__ feature
is well tested and has been shown to be useful and usable.  I think a
__nowarn__ keyword is the way to go and we can break it down to the
per-warning-flag level as a followup if there is interest by reusing DJ's
#pragma infrastructure.

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Paolo Bonzini]

> Perhaps.  The documentation says not all warnings are modifiable.  I
> didn't check whether -Wcast-qual is one of them or if this is in fact a
> bug.  IMHO because of the problems DJ noted in his summary, I believe
> using a #pragma interface is fatally flawed.  In addition to what he said,
> you also can't use #pragma in a macro definition.


Would it make sense to have some pragmas stop their effect at the end of 
the statement if they are written (as _Pragma) in the middle of the code?

Paolo

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

Paolo Bonzini <bonzini@gnu.org> writes:

| > Perhaps.  The documentation says not all warnings are modifiable.  I
| > didn't check whether -Wcast-qual is one of them or if this is in fact a
| > bug.  IMHO because of the problems DJ noted in his summary, I believe
| > using a #pragma interface is fatally flawed.  In addition to what he said,
| > you also can't use #pragma in a macro definition.
| 
| 
| Would it make sense to have some pragmas stop their effect at the end
| of the statement if they are written (as _Pragma) in the middle of the
| code?

That is far more than what we need now.  And doing what you suggest
would get again, I suspect, in the same discussion as DJ had.

-- Gaby

Re: Add a __nowarn__ keyword

[Manuel López-Ibáñez]

On 08/08/2007, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> On Wed, 8 Aug 2007, Gabriel Dos Reis wrote:
>
> > On Wed, 8 Aug 2007, Kaveh R. GHAZI wrote:
> >
> > | On Wed, 8 Aug 2007, Gabriel Dos Reis wrote:
> > |
> > | > | I am not sure whether this is a good idea since we already have the
> > | > | #pragma GCC diagnostics. But yes, the pragma does not work (yet) at
> > | > | the statement level.
> > | >
> > | > If I understand DJ correctly, they worked at the function definition level;
> > | > that is all we need.  Kaveh, did that not work?
> > | > -- Gaby
> > |
> > | I tried using the #pragma on an inline CONST_CAST function, it did not
> > | work for -Wcast-qual warnings.  I didn't bother trying to see if it worked
> > | for any others.
> > |
> > | #pragma GCC diagnostic ignored "-Wcast-qual"
> > | extern __inline void *CONST_CAST(const void *cv)
> > | {
> > |   return (void *)cv;
> > | }
> > |
> > | % gcc foo.c -Wcast-qual -c
> > | nw.c: In function 'CONST_CAST':
> > | nw.c:4: warning: cast discards qualifiers from pointer target type
> >
> > I believe you have discovered a bug in the compiler :-)
> > -- Gaby
>
> Perhaps.  The documentation says not all warnings are modifiable.  I
> didn't check whether -Wcast-qual is one of them or if this is in fact a
> bug.

The warnings (messages!) that are not modifiable are those that don't
have the first argument of warn() appropriately set. So if that
message is emitted by warn(0, "cast discards...") instead of
warn(OPT_Wcast_qual, "cast discards...") that is a bug and it will be
really good to fix it.

> In addition to what he said,
> you also can't use #pragma in a macro definition.

That is a good argument, though.

> In contrast, the keyword mechanism as shown by the __extension__ feature
> is well tested and has been shown to be useful and usable.  I think a
> __nowarn__ keyword is the way to go and we can break it down to the
> per-warning-flag level as a followup if there is interest by reusing DJ's
> #pragma infrastructure.

__nowarn__("-Wcast-qual") ? It doesn't actually look so bad, does it?

Cheers,

Manuel.

Re: Add a __nowarn__ keyword

[Daniel Jacobowitz]

On Wed, Aug 08, 2007 at 03:29:15PM +0100, Manuel López-Ibáñez wrote:
> > In contrast, the keyword mechanism as shown by the __extension__ feature
> > is well tested and has been shown to be useful and usable.  I think a
> > __nowarn__ keyword is the way to go and we can break it down to the
> > per-warning-flag level as a followup if there is interest by reusing DJ's
> > #pragma infrastructure.
> 
> __nowarn__("-Wcast-qual") ? It doesn't actually look so bad, does it?

In fact, it looks like _Pragma at that point...

-- 
Daniel Jacobowitz
CodeSourcery

Re: Add a __nowarn__ keyword

[Paolo Bonzini]

> The warnings (messages!) that are not modifiable are those that don't
> have the first argument of warn() appropriately set. So if that
> message is emitted by warn(0, "cast discards...") instead of
> warn(OPT_Wcast_qual, "cast discards...") that is a bug and it will be
> really good to fix it.

I believe these were preapproved, weren't they?

>> In addition to what he said,
>> you also can't use #pragma in a macro definition.

You can, it's _Pragma("foo").

Paolo

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Wed, 8 Aug 2007, Paolo Bonzini wrote:

> > The warnings (messages!) that are not modifiable are those that don't
> > have the first argument of warn() appropriately set. So if that
> > message is emitted by warn(0, "cast discards...") instead of
> > warn(OPT_Wcast_qual, "cast discards...") that is a bug and it will be
> > really good to fix it.
>
> I believe these were preapproved, weren't they?
>
> >> In addition to what he said,
> >> you also can't use #pragma in a macro definition.
>
> You can, it's _Pragma("foo").
> Paolo

(Ok, imagine I just had a head-smacking "Doh!" moment.)

So this is encouraging, I added OPT_Wcast_qual to the warning statement
and now it obeys the #pragma (or _Pragma).  However I'm still having one
last problem.  The pragma interface is still on/off, not push/pop.  I.e.:

	#pragma GCC diagnostic ignored "-Wcast-qual"
	extern __inline void *CONST_CAST(const void *cv)
	{
	  return (void *)cv;
	}
	//#pragma GCC diagnostic error "-Wcast-qual"

	void foo (const void *cv)
	{
	  void *v = CONST_CAST(cv);  // WARNING1
	  void *v2 = (void *)cv;  // WARNING2
	}

Assuming you've corrected the missing OPT_Wcast_qual in c-typeck.c, if you
compile the above with -Wcast-qual -Werror, the code correctly avoids the
warnings on the line marked WARNING1.  However we should still get a
warning for WARNING2 but we don't.  I believe the pragma has changed the
behavior of gcc for the rest of the entire file.  There is no "pop" at the
end of the inline function CONST_CAST.

In order to see WARNING2, I have to uncomment the second pragma.  But this
is bad IMHO because it overrides the command line.  Assuming the inline
CONST_CAST function is defined in say system.h every file that includes it
will get -Wcast-qual hardcoded on.  I don't think this is desirable for
gcc or other project that start to use this mechanism.

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

On Wed, 8 Aug 2007, Kaveh R. GHAZI wrote:

| On Wed, 8 Aug 2007, Paolo Bonzini wrote:
| 
| > > The warnings (messages!) that are not modifiable are those that don't
| > > have the first argument of warn() appropriately set. So if that
| > > message is emitted by warn(0, "cast discards...") instead of
| > > warn(OPT_Wcast_qual, "cast discards...") that is a bug and it will be
| > > really good to fix it.
| >
| > I believe these were preapproved, weren't they?
| >
| > >> In addition to what he said,
| > >> you also can't use #pragma in a macro definition.
| >
| > You can, it's _Pragma("foo").
| > Paolo
| 
| (Ok, imagine I just had a head-smacking "Doh!" moment.)
| 
| So this is encouraging, I added OPT_Wcast_qual to the warning statement
| and now it obeys the #pragma (or _Pragma).


Fantastic!

|  However I'm still having one
| last problem.  The pragma interface is still on/off, not push/pop.  I.e.:
| 
| 	#pragma GCC diagnostic ignored "-Wcast-qual"
| 	extern __inline void *CONST_CAST(const void *cv)
| 	{
| 	  return (void *)cv;
| 	}
| 	//#pragma GCC diagnostic error "-Wcast-qual"
| 
| 	void foo (const void *cv)
| 	{
| 	  void *v = CONST_CAST(cv);  // WARNING1
| 	  void *v2 = (void *)cv;  // WARNING2
| 	}
| 
| Assuming you've corrected the missing OPT_Wcast_qual in c-typeck.c, if you
| compile the above with -Wcast-qual -Werror, the code correctly avoids the
| warnings on the line marked WARNING1.  However we should still get a
| warning for WARNING2 but we don't.  I believe the pragma has changed the
| behavior of gcc for the rest of the entire file.  There is no "pop" at the
| end of the inline function CONST_CAST.

Yes, the missing "pop" is a bug.  There should be one in the machinery.

| In order to see WARNING2, I have to uncomment the second pragma.  But this
| is bad IMHO because it overrides the command line. 

I fully agree.  We should have the equivalent of

 	#pragma GCC diagnostic restore

of that CONST_CAST could be written:

 	#pragma GCC diagnostic ignored "-Wcast-qual"
 	extern __inline void *CONST_CAST(const void *cv)
 	{
 	  return (void *)cv;
 	}
 	#pragma GCC diagnostic restore

-- Gaby

Re: Add a __nowarn__ keyword

[Paolo Bonzini]

> Yes, the missing "pop" is a bug.  There should be one in the machinery.

I can see how it is by design that the flags don't reset at the end of 
the function.  But there should be support for one of

#pragma GCC diagnostic restore "-Wcast-qual"

(to restore to the cmd-line setting) or

#pragma GCC diagnostic pop "-Wcast-qual"

Paolo

Re: Add a __nowarn__ keyword

[DJ Delorie]

"Kaveh R. GHAZI" <ghazi@caip.rutgers.edu> writes:
> 	extern __inline void *CONST_CAST(const void *cv)
> 	{
> 	  return (void *)cv;
> 	}
> 	//#pragma GCC diagnostic error "-Wcast-qual"

We don't really support changing the diagnostics *after* you start
defining functions.  If that happens to work at all, it's only by
coincidence, especially with file-at-a-time optimizations and tree
inlining.

Push/pop functionality would have to be implemented along with a
scheme to actually track per-function diagnostic settings, which we
don't have.  Then we start getting into the "can I disable warnings
for this statement?" type requests.

The current design is intended to provide, for example, project-wide
defaults and policy settings, which work with things like -Wall
-Werror to tailor the global defaults to project-specfic needs, or
file-specific exceptions to them.

So, always put those pragmas before any function definitions.

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Wed, 8 Aug 2007, DJ Delorie wrote:

> "Kaveh R. GHAZI" <ghazi@caip.rutgers.edu> writes:
> > 	extern __inline void *CONST_CAST(const void *cv)
> > 	{
> > 	  return (void *)cv;
> > 	}
> > 	//#pragma GCC diagnostic error "-Wcast-qual"
>
> We don't really support changing the diagnostics *after* you start
> defining functions.  If that happens to work at all, it's only by
> coincidence, especially with file-at-a-time optimizations and tree
> inlining.
>
> Push/pop functionality would have to be implemented along with a
> scheme to actually track per-function diagnostic settings, which we
> don't have.  Then we start getting into the "can I disable warnings
> for this statement?" type requests.
>
> The current design is intended to provide, for example, project-wide
> defaults and policy settings, which work with things like -Wall
> -Werror to tailor the global defaults to project-specfic needs, or
> file-specific exceptions to them.
>
> So, always put those pragmas before any function definitions.

If you're saying that, by design, your pragma interface only allows a
once-per-file setting that must come before all function definitions, then
it isn't suited to do what I want.  I need something that allows
per-statement warning silence, then reset to whatever was before, and then
do that as many times as necessary.

In light of that, I request that either the __nowarn__ patch be accepted,
or the original union-hack patch be accepted.  I actually prefer the
union-hack without the nowarn because I'd rather not introduce a new
feature (with the inherent support issues and/or bugs) unless absolutely
necessary.  But I need one or the other.

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[DJ Delorie]

> If you're saying that, by design, your pragma interface only allows a
> once-per-file setting that must come before all function definitions, then
> it isn't suited to do what I want.  I need something that allows
> per-statement warning silence, then reset to whatever was before, and then
> do that as many times as necessary.

Right.  The current interface has two parts: The master table of what to do
with each diagnostic, and the #pragma to change the table.  What's missing
are two things:

1. A chain of "what changed" with back pointers, so we can implement
   push/pop, nested settings, and a "revert" option, and

2. A way to associate a link on that chain with a point in the program
   in a meaningful way.  It's the "meaningful way" that was hard to
   define back then.

Anyway, item 1 is relatively easy, but not useful (or at least
ill-defined) without completing #2.  #2 is hard.

Now, if a __warn__ syntax is well-defined, perhaps we could add item
#1 as a way to implement it?

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Wed, 8 Aug 2007, DJ Delorie wrote:

>
> > If you're saying that, by design, your pragma interface only allows a
> > once-per-file setting that must come before all function definitions, then
> > it isn't suited to do what I want.  I need something that allows
> > per-statement warning silence, then reset to whatever was before, and then
> > do that as many times as necessary.
>
> Right.  The current interface has two parts: The master table of what to do
> with each diagnostic, and the #pragma to change the table.  What's missing
> are two things:
>
> 1. A chain of "what changed" with back pointers, so we can implement
>    push/pop, nested settings, and a "revert" option, and
>
> 2. A way to associate a link on that chain with a point in the program
>    in a meaningful way.  It's the "meaningful way" that was hard to
>    define back then.
>
> Anyway, item 1 is relatively easy, but not useful (or at least
> ill-defined) without completing #2.  #2 is hard.
>
> Now, if a __warn__ syntax is well-defined, perhaps we could add item
> #1 as a way to implement it?

If we go with __nowarn__ (or __warn__) I believe the syntax is well
defined since it mimics the long-standing __extension__ behavior.  It
cleanly handles the push/pop paradigm because the handler code resides in
the parser.  Any time it sees the __extesion__ or __nowarn__ token, the
parser saves the current value of the warning flag, consumes the token and
recurses parsing.  When the recursion returns, it restores the old value
of the warning flag in question and proceeds.  Simple.

One potential problem is if warnings are emitted from elsewhere than in
the parsing stage.  E.g. if a warning comes from the middle-end, then
possibly __nowarn__ doesn't get to intercept it using this mechanism.
I don't know if this occurs in practice, and if so whether it happens
enough to invalidate this approach.  It sems to work for __extension__ so
I am cautiously optimistic it'll work for __nowarn__.

Still my preference is to go with the original union-hack and avoid this
altogether. :-)

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Daniel Jacobowitz]

On Wed, Aug 08, 2007 at 10:35:54PM -0400, Kaveh R. GHAZI wrote:
> If we go with __nowarn__ (or __warn__) I believe the syntax is well
> defined since it mimics the long-standing __extension__ behavior.  It
> cleanly handles the push/pop paradigm because the handler code resides in
> the parser.  Any time it sees the __extesion__ or __nowarn__ token, the
> parser saves the current value of the warning flag, consumes the token and
> recurses parsing.  When the recursion returns, it restores the old value
> of the warning flag in question and proceeds.  Simple.

Correct me if I'm wrong, but won't this work in exactly the same set
of cases where DJ's pragmas would work?  They're the cases that don't
care about his #2.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Thu, 9 Aug 2007, Daniel Jacobowitz wrote:

> On Wed, Aug 08, 2007 at 10:35:54PM -0400, Kaveh R. GHAZI wrote:
> > If we go with __nowarn__ (or __warn__) I believe the syntax is well
> > defined since it mimics the long-standing __extension__ behavior.  It
> > cleanly handles the push/pop paradigm because the handler code resides in
> > the parser.  Any time it sees the __extesion__ or __nowarn__ token, the
> > parser saves the current value of the warning flag, consumes the token and
> > recurses parsing.  When the recursion returns, it restores the old value
> > of the warning flag in question and proceeds.  Simple.
>
> Correct me if I'm wrong, but won't this work in exactly the same set
> of cases where DJ's pragmas would work?  They're the cases that don't
> care about his #2.

Maybe I don't understand your question, but the keyword vs pragma styles
clearly do not work in the same set of cases.  At least, if by "cases" you
mean: locations in code where they can be placed and affect.  If by
"cases" you mean: which warnings they can effectively silence, that I
don't know.  I think both mechanisms have holes in warning coverage that
don't necessarily overlap.

Which interpretation did you mean?

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Daniel Jacobowitz]

On Thu, Aug 09, 2007 at 10:19:27AM -0400, Kaveh R. GHAZI wrote:
> Maybe I don't understand your question, but the keyword vs pragma styles
> clearly do not work in the same set of cases.  At least, if by "cases" you
> mean: locations in code where they can be placed and affect.  If by
> "cases" you mean: which warnings they can effectively silence, that I
> don't know.  I think both mechanisms have holes in warning coverage that
> don't necessarily overlap.
> 
> Which interpretation did you mean?

All of those.  There shouldn't be anywhere you could put __nowarn__
that you couldn't put a _Pragma, since that is a preprocessing
directive, not part of the C grammar.  Nor can I see why one would
work for some warning that the other did not work for.  The natural
usage style is somewhat different, depending on how the pragma is
defined... and obviously the implementation would be very different.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Add a __nowarn__ keyword

[Manuel López-Ibáñez]

On 09/08/07, Daniel Jacobowitz <drow@false.org> wrote:
> On Thu, Aug 09, 2007 at 10:19:27AM -0400, Kaveh R. GHAZI wrote:
> > Maybe I don't understand your question, but the keyword vs pragma styles
> > clearly do not work in the same set of cases.  At least, if by "cases" you
> > mean: locations in code where they can be placed and affect.  If by
> > "cases" you mean: which warnings they can effectively silence, that I
> > don't know.  I think both mechanisms have holes in warning coverage that
> > don't necessarily overlap.
> >
> > Which interpretation did you mean?
>
> All of those.  There shouldn't be anywhere you could put __nowarn__
> that you couldn't put a _Pragma, since that is a preprocessing
> directive, not part of the C grammar.  Nor can I see why one would
> work for some warning that the other did not work for.  The natural
> usage style is somewhat different, depending on how the pragma is
> defined... and obviously the implementation would be very different.
>

There is a fundamental difference with the current implementations:
__nowarn__ does an implicit push/pop of settings around the element
(declaration, statement, etc) that follows it, while the _Pragma does
not: changed settings will keep their values until the end of the file
or next _Pragma.

Another difference is that _Pragma has no "restore" or "default"
method to restore the command-line settings. But I think that would be
easier to implement. Full push/pop capabilities, on the other hand,
would be hard (as DJ has explained above).

Also, in principle there is no reason why one would work for some
warning that the other did not work for. However, the __nowarn__ patch
is modifying inhibit_warnings which affects all warnings while _Pragma
implementation modifies the status of each named warning and it cannot
affect those messages emitted by warn(0, "whatever"). Yet,
given the current implementation, #pragma GCC diagnostics warning "-w"
should do the trick, although once you do that, you cannot disable it.

Is it any clearer now?

Cheers,

Manuel.

Re: Add a __nowarn__ keyword

[Daniel Jacobowitz]

On Thu, Aug 09, 2007 at 04:05:22PM +0100, Manuel López-Ibáñez wrote:
> Is it any clearer now?

Yes, thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Add a __nowarn__ keyword

[DJ Delorie]

> Full push/pop capabilities, on the other hand, would be hard (as DJ
> has explained above).

Obviously, I explained it poorly.

Adding push/pop to #pragma diagnostic is easy.

Semantically attaching the current state of the diagnostics to a
particular grammar construct, and having it apply consistently
throughout the various phases of the compilation, is hard.

I then conjectured that adding push/pop isn't useful until the
attachment problem is solved.

If we can solve a subset of the attachment problem, let's use the
push/pop technique I described.  It sets us up for all the things
we'll need if/when we solve the general attachment problem.

Re: Add a __nowarn__ keyword

[Mark Mitchell]

DJ Delorie wrote:

> If we can solve a subset of the attachment problem, let's use the
> push/pop technique I described.  It sets us up for all the things
> we'll need if/when we solve the general attachment problem.

I think we should add the push/pop, and then accept that it's not
perfect yet.  It's still better than nothing, and it would probably work
well for many front-end warnings, which is where most warnings come from.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Thu, 9 Aug 2007, Mark Mitchell wrote:

> DJ Delorie wrote:
>
> > If we can solve a subset of the attachment problem, let's use the
> > push/pop technique I described.  It sets us up for all the things
> > we'll need if/when we solve the general attachment problem.
>
> I think we should add the push/pop, and then accept that it's not
> perfect yet.  It's still better than nothing, and it would probably work
> well for many front-end warnings, which is where most warnings come from.
> Mark Mitchell

Based on the above, and on Ian's comments regarding warnings from the
middle-end, I think we're going to extend the pragma interface rather than
accept __nowarn__.  In this case I think DJ or someone familiar with that
code is better suited to implement the push/pop mechiansim than I.

I made a good-faith effort to provide a way for users to "elegantly" work
around -Wcast-qual warnings as Mark requested.  Since we're not going to
use it and the existing mechanism doesn't work yet, what I'd like to do is
install my CONST_CAST patch using the union.  When the pragma interface is
suitably updated, I volunteer to update the definition of CONST_CAST in
system.h to take advantage of it.  Meanwhile I can finish the
constification stuff and get GCC bootstrapping with -Wcast-qual enabled.

Mark - okay to install CONST_CAST using a union?
http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

"Kaveh R. GHAZI" <ghazi@caip.rutgers.edu> writes:

| On Thu, 9 Aug 2007, Mark Mitchell wrote:
| 
| > DJ Delorie wrote:
| >
| > > If we can solve a subset of the attachment problem, let's use the
| > > push/pop technique I described.  It sets us up for all the things
| > > we'll need if/when we solve the general attachment problem.
| >
| > I think we should add the push/pop, and then accept that it's not
| > perfect yet.  It's still better than nothing, and it would probably work
| > well for many front-end warnings, which is where most warnings come from.
| > Mark Mitchell
| 
| Based on the above, and on Ian's comments regarding warnings from the
| middle-end, I think we're going to extend the pragma interface rather than
| accept __nowarn__.  In this case I think DJ or someone familiar with that
| code is better suited to implement the push/pop mechiansim than I.
| 
| I made a good-faith effort to provide a way for users to "elegantly" work
| around -Wcast-qual warnings as Mark requested.  Since we're not going to
| use it and the existing mechanism doesn't work yet, what I'd like to do is
| install my CONST_CAST patch using the union.  When the pragma interface is
| suitably updated, I volunteer to update the definition of CONST_CAST in
| system.h to take advantage of it.  Meanwhile I can finish the
| constification stuff and get GCC bootstrapping with -Wcast-qual enabled.
| 
| Mark - okay to install CONST_CAST using a union?
| http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html

Based on the promise you made, I would suggest that we accept the
ungly union hack, and remove it as soon as possible, once we
have a good enough push/pop stuff.

-- Gaby

Re: Add a __nowarn__ keyword

[Mark Mitchell]

Kaveh R. GHAZI wrote:
> When the pragma interface is
> suitably updated, I volunteer to update the definition of CONST_CAST in
> system.h to take advantage of it.  Meanwhile I can finish the
> constification stuff and get GCC bootstrapping with -Wcast-qual enabled.
> 
> Mark - okay to install CONST_CAST using a union?
> http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html

Given your commitment to convert, yes, this is OK.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

Mark Mitchell <mark@codesourcery.com> writes:

| DJ Delorie wrote:
| 
| > If we can solve a subset of the attachment problem, let's use the
| > push/pop technique I described.  It sets us up for all the things
| > we'll need if/when we solve the general attachment problem.
| 
| I think we should add the push/pop, and then accept that it's not
| perfect yet.

I fully agree with that.

-- Gaby

Re: Add a __nowarn__ keyword

[DJ Delorie]

> I fully agree with that.

I'm working on it now.

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

On Thu, 9 Aug 2007, DJ Delorie wrote:

| 
| > I fully agree with that.
| 
| I'm working on it now.

awesome!

-- Gaby

Re: Add a __nowarn__ keyword

[DJ Delorie]

> | I'm working on it now.
> 
> awesome!

How's this look?  The classification state change tree is in the
diagnostic context, but the actual structure is opaque.  The push/pop
stack is separate, stored in c-pragma.c.  This way, the save/restore
can be used for scope-level diagnostic state once we figure out how to
define that, via a (save A, restore B, restore A) for now.  At some
point, I suppose we could pass the state pointer itself to the
diagnostic reporting functions.


Index: diagnostic.h
===================================================================
--- diagnostic.h	(revision 127325)
+++ diagnostic.h	(working copy)
@@ -53,12 +53,15 @@ typedef struct
 /*  Forward declarations.  */
 typedef struct diagnostic_context diagnostic_context;
 typedef void (*diagnostic_starter_fn) (diagnostic_context *,
 				       diagnostic_info *);
 typedef diagnostic_starter_fn diagnostic_finalizer_fn;
 
+/* The diagnostic state is an opaque pointer.  */
+struct diagnostic_classification_state_t;
+
 /* This data structure bundles altogether any information relevant to
    the context of a diagnostic message.  */
 struct diagnostic_context
 {
   /* Where most of the diagnostic formatting work is done.  */
   pretty_printer *printer;
@@ -77,12 +80,15 @@ struct diagnostic_context
      (OPT_* from options.h), this array may contain a new kind that
      the diagnostic should be changed to before reporting, or
      DK_UNSPECIFIED to leave it as the reported kind, or DK_IGNORED to
      not report it at all.  N_OPTS is from <options.h>.  */
   char classify_diagnostic[N_OPTS];
 
+  /* If non-NULL, this is the current state of changes to the above.  */
+  struct diagnostic_classification_state_t *classification_state;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
   bool abort_on_error;
@@ -188,12 +194,24 @@ extern void diagnostic_report_current_mo
 extern void diagnostic_report_current_function (diagnostic_context *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */);
+
+/* This returns a token which represents the current state of the
+   diagnostic classifications.  */
+extern struct diagnostic_classification_state_t *
+  diagnostic_save_classifications (diagnostic_context *context);
+
+/* This returns the classifications to some previous point.  Further
+   changes to the classifications will be relative to this previous
+   point.  */
+extern void diagnostic_restore_classifications (diagnostic_context *context,
+						struct diagnostic_classification_state_t *);
+
 extern void diagnostic_report_diagnostic (diagnostic_context *,
 					  diagnostic_info *);
 #ifdef ATTRIBUTE_GCC_DIAG
 extern void diagnostic_set_info (diagnostic_info *, const char *, va_list *,
 				 location_t, diagnostic_t) ATTRIBUTE_GCC_DIAG(2,0);
 extern void diagnostic_set_info_translated (diagnostic_info *, const char *,
Index: diagnostic.c
===================================================================
--- diagnostic.c	(revision 127325)
+++ diagnostic.c	(working copy)
@@ -39,12 +39,19 @@ along with GCC; see the file COPYING3.  
 #include "diagnostic.h"
 #include "langhooks.h"
 #include "langhooks-def.h"
 #include "opts.h"
 
 
+typedef struct diagnostic_classification_state_t {
+  struct diagnostic_classification_state_t *previous;
+  int option_index;
+  int kind;
+} diagnostic_classification_state_t;
+
+
 /* Prototypes.  */
 static char *build_message_string (const char *, ...) ATTRIBUTE_PRINTF_1;
 
 static void default_diagnostic_starter (diagnostic_context *,
 					diagnostic_info *);
 static void default_diagnostic_finalizer (diagnostic_context *,
@@ -53,12 +60,13 @@ static void default_diagnostic_finalizer
 static void error_recursion (diagnostic_context *) ATTRIBUTE_NORETURN;
 static bool diagnostic_count_diagnostic (diagnostic_context *,
 					 diagnostic_info *);
 static void diagnostic_action_after_output (diagnostic_context *,
 					    diagnostic_info *);
 static void real_abort (void) ATTRIBUTE_NORETURN;
+static int lookup_classification (diagnostic_context *, int);
 
 /* A diagnostic_context surrogate for stderr.  */
 static diagnostic_context global_diagnostic_context;
 diagnostic_context *global_dc = &global_diagnostic_context;
 
 \f
@@ -325,17 +333,75 @@ diagnostic_classify_diagnostic (diagnost
 
   if (option_index <= 0
       || option_index >= N_OPTS
       || new_kind >= DK_LAST_DIAGNOSTIC_KIND)
     return DK_UNSPECIFIED;
 
+  if (context->classification_state)
+    {
+      struct diagnostic_classification_state_t *state;
+
+      old_kind = lookup_classification (context, option_index);
+      state = (struct diagnostic_classification_state_t *)
+	xmalloc (sizeof (struct diagnostic_classification_state_t));
+
+      state->previous = context->classification_state;
+      state->option_index = option_index;
+      state->kind = new_kind;
+      context->classification_state = state;
+
+      return old_kind;
+    }
+
   old_kind = context->classify_diagnostic[option_index];
   context->classify_diagnostic[option_index] = new_kind;
   return old_kind;
 }
 
+static int
+lookup_classification (diagnostic_context * context, int option_index)
+{
+  if (option_index <= 0
+      || option_index >= N_OPTS)
+    return DK_UNSPECIFIED;
+
+  if (context->classification_state)
+    {
+      diagnostic_classification_state_t *s;
+      for (s = context->classification_state;
+	   s; s = s->previous)
+	{
+	  if (s->option_index == option_index)
+	    return s->kind;
+	}
+    }
+  return context->classify_diagnostic[option_index];
+}
+
+struct diagnostic_classification_state_t *
+diagnostic_save_classifications (diagnostic_context *context)
+{
+  if (context->classification_state == NULL)
+    {
+      /* Create the head of the chain.  */
+      context->classification_state = (struct diagnostic_classification_state_t *)
+	xmalloc (sizeof (struct diagnostic_classification_state_t));
+      context->classification_state->previous = NULL;
+      context->classification_state->option_index = 0;
+      context->classification_state->kind = context->classify_diagnostic[0];
+    }
+  return context->classification_state;
+}
+
+void
+diagnostic_restore_classifications (diagnostic_context *context,
+				    struct diagnostic_classification_state_t *state)
+{
+  context->classification_state = state;
+}
+
 /* Report a diagnostic message (an error or a warning) as specified by
    DC.  This function is *the* subroutine in terms of which front-ends
    should implement their specific diagnostic handling modules.  The
    front-end independent format specifiers are exactly those described
    in the documentation of output_format.  */
 
@@ -372,21 +438,23 @@ diagnostic_report_diagnostic (diagnostic
       diagnostic->kind = DK_ERROR;
       maybe_print_warnings_as_errors_message = true;
     }
   
   if (diagnostic->option_index)
     {
+      int kind;
       /* This tests if the user provided the appropriate -Wfoo or
 	 -Wno-foo option.  */
       if (! option_enabled (diagnostic->option_index))
 	return;
       /* This tests if the user provided the appropriate -Werror=foo
 	 option.  */
-      if (context->classify_diagnostic[diagnostic->option_index] != DK_UNSPECIFIED)
+      kind = lookup_classification (context, diagnostic->option_index);
+      if (kind != DK_UNSPECIFIED)
 	{
-	  diagnostic->kind = context->classify_diagnostic[diagnostic->option_index];
+	  diagnostic->kind = kind;
 	  maybe_print_warnings_as_errors_message = false;
 	}
       /* This allows for future extensions, like temporarily disabling
 	 warnings for ranges of source code.  */
       if (diagnostic->kind == DK_IGNORED)
 	return;
Index: c-pragma.c
===================================================================
--- c-pragma.c	(revision 127325)
+++ c-pragma.c	(working copy)
@@ -814,31 +814,62 @@ handle_pragma_visibility (cpp_reader *du
   if (pragma_lex (&x) != CPP_EOF)
     warning (OPT_Wpragmas, "junk at end of %<#pragma GCC visibility%>");
 }
 
 #endif
 
+typedef struct diagnostic_stack_entry_t {
+  struct diagnostic_stack_entry_t *previous;
+  struct diagnostic_classification_state_t *state;
+} diagnostic_stack_entry_t;
+
+static diagnostic_stack_entry_t *diagnostic_stack = 0;
+
 static void
 handle_pragma_diagnostic(cpp_reader *ARG_UNUSED(dummy))
 {
   const char *kind_string, *option_string;
   unsigned int option_index;
   enum cpp_ttype token;
   diagnostic_t kind;
   tree x;
+  diagnostic_stack_entry_t *stack;
 
   if (cfun)
     {
       error ("#pragma GCC diagnostic not allowed inside functions");
       return;
     }
 
   token = pragma_lex (&x);
   if (token != CPP_NAME)
     GCC_BAD ("missing [error|warning|ignored] after %<#pragma GCC diagnostic%>");
   kind_string = IDENTIFIER_POINTER (x);
+
+  if (strcmp (kind_string, "push") == 0)
+    {
+      stack = (diagnostic_stack_entry_t *) xmalloc (sizeof (diagnostic_stack_entry_t));
+      stack->previous = diagnostic_stack;
+      stack->state = diagnostic_save_classifications (global_dc);
+      diagnostic_stack = stack;
+      return;
+    }
+  else if (strcmp (kind_string, "pop") == 0)
+    {
+      stack = diagnostic_stack;
+      if (!stack)
+	{
+	  error ("$pragma GCC diagnostic pop with no matching push");
+	  return;
+	}
+      diagnostic_stack = stack->previous;
+      diagnostic_restore_classifications (global_dc, stack->state);
+      free (stack);
+      return;
+    }
+
   if (strcmp (kind_string, "error") == 0)
     kind = DK_ERROR;
   else if (strcmp (kind_string, "warning") == 0)
     kind = DK_WARNING;
   else if (strcmp (kind_string, "ignored") == 0)
     kind = DK_IGNORED;

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

On Thu, 9 Aug 2007, DJ Delorie wrote:

| 
| > | I'm working on it now.
| > 
| > awesome!
| 
| How's this look?  The classification state change tree is in the
| diagnostic context, but the actual structure is opaque.  The push/pop
| stack is separate, stored in c-pragma.c.  This way, the save/restore
| can be used for scope-level diagnostic state once we figure out how to
| define that, via a (save A, restore B, restore A) for now.  At some
| point, I suppose we could pass the state pointer itself to the
| diagnostic reporting functions.


Looks good.

Stylistic note:  please could you add a 

  typedef struct diagnostic_classification_state_t
             diagnostic_classification_state_t;

and use the typedef instead?


Is it correct that the "user interface" now is to say
  
    #pragma GCC push
    #pragma GCC diagnostics ignore "-Wcast-qual

    ....

   #pragma GCC pop

?

If yes, is the "push" necessary?  Can we make it implicit?


Many thanks for working on this.

-- Gaby

Re: Add a __nowarn__ keyword

[DJ Delorie]

> and use the typedef instead?

What, you don't like all those "structs" around?  ;-)

> Is it correct that the "user interface" now is to say
>   
>     #pragma GCC push

$pragma GCC diagnostics push

> ?

Yes.

> If yes, is the "push" necessary?  Can we make it implicit?

So, a pop with no corresponding push, restores to... what?  The way
the code is now, if you don't push or save, it never even starts the
chain - it doesn't know the difference between a command line setting
and a #pragma without a push.  Thus, a pop wouldn't do anything
anyway.

I think what you want is, an unmatched pop restores the command line
settings, yes?  To do that, we'd need two things:

1. Something in gcc does the implicit save to start the chain, so that
   all #pragma changes are in the chain and not the table.

2. An unmatched pop points you back at the head of the chain, which is
   a no-op state created by that save.

But then, there would be no way to restore some project-wide settings
pulled in by a #include.

Or did you mean that a pop always pops just ONE #pragma?  The current
structure allows that, easier if we move the state struct to
diagnostics.h.  Then, you just restore to state->previous.  But is
that a pop, or an undo?

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

On Thu, 9 Aug 2007, DJ Delorie wrote:

| 
| > and use the typedef instead?
| 
| What, you don't like all those "structs" around?  ;-)

:-) :-)

As you probably know by now, my brain has been altered by C++ :-)

| > Is it correct that the "user interface" now is to say
| >   
| >     #pragma GCC push
| 
| $pragma GCC diagnostics push
| 
| > ?
| 
| Yes.
| 
| > If yes, is the "push" necessary?  Can we make it implicit?
| 
| So, a pop with no corresponding push, restores to... what?  The way
| the code is now, if you don't push or save, it never even starts the
| chain - it doesn't know the difference between a command line setting
| and a #pragma without a push.  Thus, a pop wouldn't do anything
| anyway.
| 
| I think what you want is, an unmatched pop restores the command line
| settings, yes?  To do that, we'd need two things:
| 
| 1. Something in gcc does the implicit save to start the chain, so that
|    all #pragma changes are in the chain and not the table.
| 
| 2. An unmatched pop points you back at the head of the chain, which is
|    a no-op state created by that save.
| 
| But then, there would be no way to restore some project-wide settings
| pulled in by a #include.
| 
| Or did you mean that a pop always pops just ONE #pragma?  The current
| structure allows that, easier if we move the state struct to
| diagnostics.h.  Then, you just restore to state->previous.  But is
| that a pop, or an undo?


What I have in mind is something like this:

  1. When we see

            #pragma GCC diagnostics foo

     we implicitly save thre current state, push a new one and modify
     that copy to match the requested setting foo.

  2.  When we see

            #pragma GCC diagnostics restore


      we pop the stack and restore the state to the previously known
      state.

Is your point that that scheme is too simplistic to cover all we
want or we would like to support?


-- Gaby

Re: Add a __nowarn__ keyword

[DJ Delorie]

> Is your point that that scheme is too simplistic to cover all we
> want or we would like to support?

Too simple.  Imagine a project-wide central header with dozens of
#pragmas that set up the warnings for the project.  Imagine lexical
scopes with multiple pragmas in them.  Imagine headers that want
stricter diagnostics, but need to preserve the state they started
with.

I think you want "undo", which undoes the most recent #pragma.  We can
add this along with the push/pop, but we're back to the original
question: Where/when does the state chain start?  Currently, we don't
start a chain until we see a push, so that if we don't use push/pop we
don't have to traverse the chain for every reported diagnostic.

If we implement undo, we're basically throwing that away, and starting
a chain with the first #pragma.

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

On Fri, 10 Aug 2007, DJ Delorie wrote:

| I think you want "undo", which undoes the most recent #pragma. 

Yes, that is right.

| We can
| add this along with the push/pop, but we're back to the original
| question: Where/when does the state chain start?  Currently, we don't
| start a chain until we see a push, so that if we don't use push/pop we
| don't have to traverse the chain for every reported diagnostic.

OK; I believe you convinced me to go with push/pop now.

Thanks,

-- Gaby

Re: Add a __nowarn__ keyword

[DJ Delorie]

Minor glitch...

The #pragma to enable the diagnostic also enables the warn_foo flag
that goes with it, so if we have code like:

	if (warn_foo)
	   warning (OPT_foo, "...");

then we need to make sure the warning function is called.  However,
when we pop state, we don't restore all those warn_foo flags.  So,
popping state may result in some warnings showing up as "warning:
foo".

This happens because the default state in diagnostics.c is
DK_UNSPECIFIED, not DK_IGNORED.  We do this so that the call itself
tells us the default; calls to warning() generate warnings, etc.

Ideas?

The only thing I can think of is if we're turning the option on when
it was off, we can go into the classification table and tweak that
entry to DK_IGNORED, but that assumes that all warning() calls using
that option pass the right OPT_*.

Re: Add a __nowarn__ keyword

[Joseph S. Myers]

On Fri, 10 Aug 2007, DJ Delorie wrote:

> Minor glitch...
> 
> The #pragma to enable the diagnostic also enables the warn_foo flag
> that goes with it, so if we have code like:
> 
> 	if (warn_foo)
> 	   warning (OPT_foo, "...");
> 
> then we need to make sure the warning function is called.  However,
> when we pop state, we don't restore all those warn_foo flags.  So,
> popping state may result in some warnings showing up as "warning:
> foo".
> 
> This happens because the default state in diagnostics.c is
> DK_UNSPECIFIED, not DK_IGNORED.  We do this so that the call itself
> tells us the default; calls to warning() generate warnings, etc.
> 
> Ideas?
> 
> The only thing I can think of is if we're turning the option on when
> it was off, we can go into the classification table and tweak that
> entry to DK_IGNORED, but that assumes that all warning() calls using
> that option pass the right OPT_*.

Eventually, when the pragma states are linked to ranges of source 
locations so that diagnostics issued after the front end can be correctly 
handled (mapped-location might help here), and all warning calls pass a 
suitable OPT_*, warn_foo should be on permanently once it's been turned on 
anywhere (so that blocks of code like the example you give conditional on 
warn_foo are executed whenever they might be useful), and it would be 
entirely for the diagnostic machinery to tell whether a warning such code 
issues is to be output at the given source location.  But I'm not sure 
what's best for now until we have the link to source location ranges and a 
full conversion to OPT_*.  (I suspect restoring the warn_foo variables 
when state is popped, but I don't know how hard that is.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Add a __nowarn__ keyword

[DJ Delorie]

> Eventually, when the pragma states are linked to ranges of source 
> locations so that diagnostics issued after the front end can be correctly 
> handled (mapped-location might help here),

I can certainly store the mapped-location in the classification tree,
and look up the state as of that mapping.  Is that universally
available to the diagnostics machinery?

> full conversion to OPT_*.  (I suspect restoring the warn_foo variables 
> when state is popped, but I don't know how hard that is.)

The pragma only deals with options passed with OPT_* anyway.  I
suppose what we can do at the moment is check to see if the initial
table has "unspecified", and if so, set it to ignore, warn, or error
as appropriate.

Re: Add a __nowarn__ keyword

[DJ Delorie]

Patch so far.  I added some code to fill in the origin array in the
global_dc to what the switch "means" before we go and change it, so
that using the table results in the same effect as if we had never
changed the switch.

I haven't tried to do anything with the mapped-location in the source.
Recording the location is easy (I think), but to take advantage of it
we'd probably have to restructure the classification tree some so that
we can search it.  Plus, there may be minor changes to the diagnostic
API to pass the current location to the diagnostics module.  Unless we
already think the location information we have access to is reliable?
The diagnostics do print the file and line with each message...



Index: diagnostic.c
===================================================================
--- diagnostic.c	(revision 127658)
+++ diagnostic.c	(working copy)
@@ -53,12 +53,13 @@ static void default_diagnostic_finalizer
 static void error_recursion (diagnostic_context *) ATTRIBUTE_NORETURN;
 static bool diagnostic_count_diagnostic (diagnostic_context *,
 					 diagnostic_info *);
 static void diagnostic_action_after_output (diagnostic_context *,
 					    diagnostic_info *);
 static void real_abort (void) ATTRIBUTE_NORETURN;
+static int lookup_classification (diagnostic_context *, int);
 
 /* A diagnostic_context surrogate for stderr.  */
 static diagnostic_context global_diagnostic_context;
 diagnostic_context *global_dc = &global_diagnostic_context;
 
 \f
@@ -325,17 +326,75 @@ diagnostic_classify_diagnostic (diagnost
 
   if (option_index <= 0
       || option_index >= N_OPTS
       || new_kind >= DK_LAST_DIAGNOSTIC_KIND)
     return DK_UNSPECIFIED;
 
+  if (context->classification_state)
+    {
+      diagnostic_classification_state_t *state;
+
+      old_kind = lookup_classification (context, option_index);
+      state = (diagnostic_classification_state_t *)
+	xmalloc (sizeof (diagnostic_classification_state_t));
+
+      state->previous = context->classification_state;
+      state->option_index = option_index;
+      state->kind = new_kind;
+      context->classification_state = state;
+
+      return old_kind;
+    }
+
   old_kind = context->classify_diagnostic[option_index];
   context->classify_diagnostic[option_index] = new_kind;
   return old_kind;
 }
 
+static int
+lookup_classification (diagnostic_context * context, int option_index)
+{
+  if (option_index <= 0
+      || option_index >= N_OPTS)
+    return DK_UNSPECIFIED;
+
+  if (context->classification_state)
+    {
+      diagnostic_classification_state_t *s;
+      for (s = context->classification_state;
+	   s; s = s->previous)
+	{
+	  if (s->option_index == option_index)
+	    return s->kind;
+	}
+    }
+  return context->classify_diagnostic[option_index];
+}
+
+diagnostic_classification_state_t *
+diagnostic_save_classifications (diagnostic_context *context)
+{
+  if (context->classification_state == NULL)
+    {
+      /* Create the head of the chain.  */
+      context->classification_state = (diagnostic_classification_state_t *)
+	xmalloc (sizeof (diagnostic_classification_state_t));
+      context->classification_state->previous = NULL;
+      context->classification_state->option_index = 0;
+      context->classification_state->kind = context->classify_diagnostic[0];
+    }
+  return context->classification_state;
+}
+
+void
+diagnostic_restore_classifications (diagnostic_context *context,
+				    diagnostic_classification_state_t *state)
+{
+  context->classification_state = state;
+}
+
 /* Report a diagnostic message (an error or a warning) as specified by
    DC.  This function is *the* subroutine in terms of which front-ends
    should implement their specific diagnostic handling modules.  The
    front-end independent format specifiers are exactly those described
    in the documentation of output_format.  */
 
@@ -372,21 +431,23 @@ diagnostic_report_diagnostic (diagnostic
       diagnostic->kind = DK_ERROR;
       maybe_print_warnings_as_errors_message = true;
     }
   
   if (diagnostic->option_index)
     {
+      int kind;
       /* This tests if the user provided the appropriate -Wfoo or
 	 -Wno-foo option.  */
       if (! option_enabled (diagnostic->option_index))
 	return;
       /* This tests if the user provided the appropriate -Werror=foo
 	 option.  */
-      if (context->classify_diagnostic[diagnostic->option_index] != DK_UNSPECIFIED)
+      kind = lookup_classification (context, diagnostic->option_index);
+      if (kind != DK_UNSPECIFIED)
 	{
-	  diagnostic->kind = context->classify_diagnostic[diagnostic->option_index];
+	  diagnostic->kind = kind;
 	  maybe_print_warnings_as_errors_message = false;
 	}
       /* This allows for future extensions, like temporarily disabling
 	 warnings for ranges of source code.  */
       if (diagnostic->kind == DK_IGNORED)
 	return;
Index: diagnostic.h
===================================================================
--- diagnostic.h	(revision 127658)
+++ diagnostic.h	(working copy)
@@ -53,12 +53,19 @@ typedef struct
 /*  Forward declarations.  */
 typedef struct diagnostic_context diagnostic_context;
 typedef void (*diagnostic_starter_fn) (diagnostic_context *,
 				       diagnostic_info *);
 typedef diagnostic_starter_fn diagnostic_finalizer_fn;
 
+/* The diagnostic state is an opaque pointer.  */
+typedef struct diagnostic_classification_state_t {
+  struct diagnostic_classification_state_t *previous;
+  int option_index;
+  int kind;
+} diagnostic_classification_state_t;
+
 /* This data structure bundles altogether any information relevant to
    the context of a diagnostic message.  */
 struct diagnostic_context
 {
   /* Where most of the diagnostic formatting work is done.  */
   pretty_printer *printer;
@@ -77,12 +84,15 @@ struct diagnostic_context
      (OPT_* from options.h), this array may contain a new kind that
      the diagnostic should be changed to before reporting, or
      DK_UNSPECIFIED to leave it as the reported kind, or DK_IGNORED to
      not report it at all.  N_OPTS is from <options.h>.  */
   char classify_diagnostic[N_OPTS];
 
+  /* If non-NULL, this is the current state of changes to the above.  */
+  diagnostic_classification_state_t *classification_state;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
   bool abort_on_error;
@@ -188,12 +198,24 @@ extern void diagnostic_report_current_mo
 extern void diagnostic_report_current_function (diagnostic_context *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */);
+
+/* This returns a token which represents the current state of the
+   diagnostic classifications.  */
+extern diagnostic_classification_state_t *
+  diagnostic_save_classifications (diagnostic_context *context);
+
+/* This returns the classifications to some previous point.  Further
+   changes to the classifications will be relative to this previous
+   point.  */
+extern void diagnostic_restore_classifications (diagnostic_context *context,
+						diagnostic_classification_state_t *);
+
 extern void diagnostic_report_diagnostic (diagnostic_context *,
 					  diagnostic_info *);
 #ifdef ATTRIBUTE_GCC_DIAG
 extern void diagnostic_set_info (diagnostic_info *, const char *, va_list *,
 				 location_t, diagnostic_t) ATTRIBUTE_GCC_DIAG(2,0);
 extern void diagnostic_set_info_translated (diagnostic_info *, const char *,
Index: c-pragma.c
===================================================================
--- c-pragma.c	(revision 127658)
+++ c-pragma.c	(working copy)
@@ -814,39 +814,91 @@ handle_pragma_visibility (cpp_reader *du
   if (pragma_lex (&x) != CPP_EOF)
     warning (OPT_Wpragmas, "junk at end of %<#pragma GCC visibility%>");
 }
 
 #endif
 
+typedef struct diagnostic_stack_entry_t {
+  struct diagnostic_stack_entry_t *previous;
+  struct diagnostic_classification_state_t *state;
+} diagnostic_stack_entry_t;
+
+static diagnostic_stack_entry_t *diagnostic_stack = 0;
+
 static void
 handle_pragma_diagnostic(cpp_reader *ARG_UNUSED(dummy))
 {
   const char *kind_string, *option_string;
   unsigned int option_index;
   enum cpp_ttype token;
   diagnostic_t kind;
   tree x;
+  diagnostic_stack_entry_t *stack;
 
   if (cfun)
     {
       error ("#pragma GCC diagnostic not allowed inside functions");
       return;
     }
 
   token = pragma_lex (&x);
   if (token != CPP_NAME)
-    GCC_BAD ("missing [error|warning|ignored] after %<#pragma GCC diagnostic%>");
+    GCC_BAD ("missing [push|pop|error|warning|ignored] after %<#pragma GCC diagnostic%>");
   kind_string = IDENTIFIER_POINTER (x);
+
+  if (strcmp (kind_string, "push") == 0)
+    {
+      stack = (diagnostic_stack_entry_t *) xmalloc (sizeof (diagnostic_stack_entry_t));
+      stack->previous = diagnostic_stack;
+      stack->state = diagnostic_save_classifications (global_dc);
+      diagnostic_stack = stack;
+ 
+      token = pragma_lex (&x);
+      if (token != CPP_NAME)
+	return;
+      kind_string = IDENTIFIER_POINTER (x);
+    }
+  else if (strcmp (kind_string, "pop") == 0)
+    {
+      stack = diagnostic_stack;
+      if (!stack)
+	{
+	  error ("$pragma GCC diagnostic pop with no matching push");
+	  return;
+	}
+      diagnostic_stack = stack->previous;
+      diagnostic_restore_classifications (global_dc, stack->state);
+      free (stack);
+      return;
+    }
+#if 0
+  /* This is an example of how to do this, in case we decide to need
+     this functionality in the future.  "undo" moves one state up the
+     tree, effectively undoing the previous pragma.  It relies on
+     every pragma being in the state tree (which may be expensive), so
+     we must force the tree to exist somehow.  */
+  else if (strcmp (kind_string, "undo") == 0)
+    {
+      diagnostic_classification_state_t *state;
+      state = diagnostic_save_classifications (global_dc);
+      state = state->previous;
+      diagnostic_restore_classifications (global_dc, state);
+      return;
+    }
+  /* Here we force the state tree to exist for any change-type pragma.  */
+  diagnostic_save_classifications (global_dc);
+#endif
+
   if (strcmp (kind_string, "error") == 0)
     kind = DK_ERROR;
   else if (strcmp (kind_string, "warning") == 0)
     kind = DK_WARNING;
   else if (strcmp (kind_string, "ignored") == 0)
     kind = DK_IGNORED;
   else
-    GCC_BAD ("expected [error|warning|ignored] after %<#pragma GCC diagnostic%>");
+    GCC_BAD ("expected [push|pop|error|warning|ignored] after %<#pragma GCC diagnostic%>");
 
   token = pragma_lex (&x);
   if (token != CPP_STRING)
     GCC_BAD ("missing option after %<#pragma GCC diagnostic%> kind");
   option_string = TREE_STRING_POINTER (x);
   for (option_index = 0; option_index < cl_options_count; option_index++)
@@ -855,13 +907,29 @@ handle_pragma_diagnostic(cpp_reader *ARG
 	/* This overrides -Werror, for example.  */
 	diagnostic_classify_diagnostic (global_dc, option_index, kind);
 	/* This makes sure the option is enabled, like -Wfoo would do.  */
 	if (cl_options[option_index].var_type == CLVC_BOOLEAN
 	    && cl_options[option_index].flag_var
 	    && kind != DK_IGNORED)
+	  {
+	    /* Make the default setting explicit, in case we pop.  */
+	    if (global_dc->classify_diagnostic[option_index] == DK_UNSPECIFIED)
+	      {
+		if (*(int *) cl_options[option_index].flag_var)
+		  {
+		    if (global_dc->warning_as_error_requested)
+		      global_dc->classify_diagnostic[option_index] = DK_ERROR;
+		    else
+		      global_dc->classify_diagnostic[option_index] = DK_WARNING;
+		  }
+		else
+		  global_dc->classify_diagnostic[option_index] = DK_IGNORED;
+	      }
+	    /* And turn the option on.  */
 	    *(int *) cl_options[option_index].flag_var = 1;
+	  }
 	return;
       }
   GCC_BAD ("unknown option after %<#pragma GCC diagnostic%> kind");
 }
 
 /* A vector of registered pragma callbacks.  */

Re: Add a __nowarn__ keyword

[Magnus Fromreide]

Whatever happened to the #pragma GCC diagnostic push/pop patch? From the
mailing list it sounded as if it was destined to be added but then
everything just disappeared.

/MF (Interested end user wanting to disable a warning in a stupid
     interface temporarily)

Re: Add a __nowarn__ keyword

[Manuel López-Ibáñez]

It is not clear how we want to implement this: as a pragma, as an
attribute or a special keyword similar to __extension__. (Although the
pragma was the clear winner last time I looked at this). Each of those
have benefits and disadvantages. It depends what you actually want to
do with it, and also their actual capabilities and shortcomings depend
on current limitations of the diagnostics machinery. On the bright
side, recent changes to the diagnostics machinery and accuracy of
location information probably have improved the situation.

I guess also that people involved had better things to do, and there
has not been a lot of interest from users. We don't even know whether
people are using the current "#pragma GCC diagnostic" interface. In
any case, GCC 4.5 is in bug-fixing mode, so this would have to wait
until GCC 4.6.

If you wish to speed up things, take the lastest patch [*], update it
to current trunk and start testing it. You may consider submit it
yourself once 4.5 is released: http://gcc.gnu.org/contribute.html

Cheers,

Manuel.

[*] http://gcc.gnu.org/ml/gcc-patches/2007-08/msg00702.html

2009/11/21 Magnus Fromreide <magfr@lysator.liu.se>:
> Whatever happened to the #pragma GCC diagnostic push/pop patch? From the
> mailing list it sounded as if it was destined to be added but then
> everything just disappeared.
>
> /MF (Interested end user wanting to disable a warning in a stupid
>     interface temporarily)
>
>

Re: Add a __nowarn__ keyword

[DJ Delorie]

> Whatever happened to the #pragma GCC diagnostic push/pop patch? From
> the mailing list it sounded as if it was destined to be added but
> then everything just disappeared.

If you're talking about the work I did, at the time I was working on
it, I needed the new "location pointer" code (Pretty sure that's not
the right name).  By the time that functionality was added and made
the default, I was on to another project and didn't have time to go
back and update the patch.

The basic idea was that the diagnostic machinery would keep track of
the line numbers for each #pragma, so it could keep various groups of
settings and find the right group based on the line number that the
diagnostic was generated by.  That would only work if we had a
"location" which was a single monotonic value relative to the
preprocessed source, so we could easily and accurately search the
"source space".

So, each push made a copy of the settings data and noted the location.
Each pop noted the location and went back to the previous data.  Each
diagnostic finds the right data set and uses it.

Re: Add a __nowarn__ keyword

[DJ Delorie]

Here's the latest patch, with push/pop, push+change, and undo
(commented out).

Index: diagnostic.h
===================================================================
--- diagnostic.h	(revision 127330)
+++ diagnostic.h	(working copy)
@@ -53,12 +53,19 @@ typedef struct
 /*  Forward declarations.  */
 typedef struct diagnostic_context diagnostic_context;
 typedef void (*diagnostic_starter_fn) (diagnostic_context *,
 				       diagnostic_info *);
 typedef diagnostic_starter_fn diagnostic_finalizer_fn;
 
+/* The diagnostic state is an opaque pointer.  */
+typedef struct diagnostic_classification_state_t {
+  struct diagnostic_classification_state_t *previous;
+  int option_index;
+  int kind;
+} diagnostic_classification_state_t;
+
 /* This data structure bundles altogether any information relevant to
    the context of a diagnostic message.  */
 struct diagnostic_context
 {
   /* Where most of the diagnostic formatting work is done.  */
   pretty_printer *printer;
@@ -77,12 +84,15 @@ struct diagnostic_context
      (OPT_* from options.h), this array may contain a new kind that
      the diagnostic should be changed to before reporting, or
      DK_UNSPECIFIED to leave it as the reported kind, or DK_IGNORED to
      not report it at all.  N_OPTS is from <options.h>.  */
   char classify_diagnostic[N_OPTS];
 
+  /* If non-NULL, this is the current state of changes to the above.  */
+  diagnostic_classification_state_t *classification_state;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
   bool abort_on_error;
@@ -188,12 +198,24 @@ extern void diagnostic_report_current_mo
 extern void diagnostic_report_current_function (diagnostic_context *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */);
+
+/* This returns a token which represents the current state of the
+   diagnostic classifications.  */
+extern diagnostic_classification_state_t *
+  diagnostic_save_classifications (diagnostic_context *context);
+
+/* This returns the classifications to some previous point.  Further
+   changes to the classifications will be relative to this previous
+   point.  */
+extern void diagnostic_restore_classifications (diagnostic_context *context,
+						diagnostic_classification_state_t *);
+
 extern void diagnostic_report_diagnostic (diagnostic_context *,
 					  diagnostic_info *);
 #ifdef ATTRIBUTE_GCC_DIAG
 extern void diagnostic_set_info (diagnostic_info *, const char *, va_list *,
 				 location_t, diagnostic_t) ATTRIBUTE_GCC_DIAG(2,0);
 extern void diagnostic_set_info_translated (diagnostic_info *, const char *,
Index: diagnostic.c
===================================================================
--- diagnostic.c	(revision 127330)
+++ diagnostic.c	(working copy)
@@ -53,12 +53,13 @@ static void default_diagnostic_finalizer
 static void error_recursion (diagnostic_context *) ATTRIBUTE_NORETURN;
 static bool diagnostic_count_diagnostic (diagnostic_context *,
 					 diagnostic_info *);
 static void diagnostic_action_after_output (diagnostic_context *,
 					    diagnostic_info *);
 static void real_abort (void) ATTRIBUTE_NORETURN;
+static int lookup_classification (diagnostic_context *, int);
 
 /* A diagnostic_context surrogate for stderr.  */
 static diagnostic_context global_diagnostic_context;
 diagnostic_context *global_dc = &global_diagnostic_context;
 
 \f
@@ -325,17 +326,75 @@ diagnostic_classify_diagnostic (diagnost
 
   if (option_index <= 0
       || option_index >= N_OPTS
       || new_kind >= DK_LAST_DIAGNOSTIC_KIND)
     return DK_UNSPECIFIED;
 
+  if (context->classification_state)
+    {
+      diagnostic_classification_state_t *state;
+
+      old_kind = lookup_classification (context, option_index);
+      state = (diagnostic_classification_state_t *)
+	xmalloc (sizeof (diagnostic_classification_state_t));
+
+      state->previous = context->classification_state;
+      state->option_index = option_index;
+      state->kind = new_kind;
+      context->classification_state = state;
+
+      return old_kind;
+    }
+
   old_kind = context->classify_diagnostic[option_index];
   context->classify_diagnostic[option_index] = new_kind;
   return old_kind;
 }
 
+static int
+lookup_classification (diagnostic_context * context, int option_index)
+{
+  if (option_index <= 0
+      || option_index >= N_OPTS)
+    return DK_UNSPECIFIED;
+
+  if (context->classification_state)
+    {
+      diagnostic_classification_state_t *s;
+      for (s = context->classification_state;
+	   s; s = s->previous)
+	{
+	  if (s->option_index == option_index)
+	    return s->kind;
+	}
+    }
+  return context->classify_diagnostic[option_index];
+}
+
+diagnostic_classification_state_t *
+diagnostic_save_classifications (diagnostic_context *context)
+{
+  if (context->classification_state == NULL)
+    {
+      /* Create the head of the chain.  */
+      context->classification_state = (diagnostic_classification_state_t *)
+	xmalloc (sizeof (diagnostic_classification_state_t));
+      context->classification_state->previous = NULL;
+      context->classification_state->option_index = 0;
+      context->classification_state->kind = context->classify_diagnostic[0];
+    }
+  return context->classification_state;
+}
+
+void
+diagnostic_restore_classifications (diagnostic_context *context,
+				    diagnostic_classification_state_t *state)
+{
+  context->classification_state = state;
+}
+
 /* Report a diagnostic message (an error or a warning) as specified by
    DC.  This function is *the* subroutine in terms of which front-ends
    should implement their specific diagnostic handling modules.  The
    front-end independent format specifiers are exactly those described
    in the documentation of output_format.  */
 
@@ -372,21 +431,23 @@ diagnostic_report_diagnostic (diagnostic
       diagnostic->kind = DK_ERROR;
       maybe_print_warnings_as_errors_message = true;
     }
   
   if (diagnostic->option_index)
     {
+      int kind;
       /* This tests if the user provided the appropriate -Wfoo or
 	 -Wno-foo option.  */
       if (! option_enabled (diagnostic->option_index))
 	return;
       /* This tests if the user provided the appropriate -Werror=foo
 	 option.  */
-      if (context->classify_diagnostic[diagnostic->option_index] != DK_UNSPECIFIED)
+      kind = lookup_classification (context, diagnostic->option_index);
+      if (kind != DK_UNSPECIFIED)
 	{
-	  diagnostic->kind = context->classify_diagnostic[diagnostic->option_index];
+	  diagnostic->kind = kind;
 	  maybe_print_warnings_as_errors_message = false;
 	}
       /* This allows for future extensions, like temporarily disabling
 	 warnings for ranges of source code.  */
       if (diagnostic->kind == DK_IGNORED)
 	return;
Index: c-pragma.c
===================================================================
--- c-pragma.c	(revision 127330)
+++ c-pragma.c	(working copy)
@@ -814,39 +814,91 @@ handle_pragma_visibility (cpp_reader *du
   if (pragma_lex (&x) != CPP_EOF)
     warning (OPT_Wpragmas, "junk at end of %<#pragma GCC visibility%>");
 }
 
 #endif
 
+typedef struct diagnostic_stack_entry_t {
+  struct diagnostic_stack_entry_t *previous;
+  struct diagnostic_classification_state_t *state;
+} diagnostic_stack_entry_t;
+
+static diagnostic_stack_entry_t *diagnostic_stack = 0;
+
 static void
 handle_pragma_diagnostic(cpp_reader *ARG_UNUSED(dummy))
 {
   const char *kind_string, *option_string;
   unsigned int option_index;
   enum cpp_ttype token;
   diagnostic_t kind;
   tree x;
+  diagnostic_stack_entry_t *stack;
 
   if (cfun)
     {
       error ("#pragma GCC diagnostic not allowed inside functions");
       return;
     }
 
   token = pragma_lex (&x);
   if (token != CPP_NAME)
     GCC_BAD ("missing [error|warning|ignored] after %<#pragma GCC diagnostic%>");
   kind_string = IDENTIFIER_POINTER (x);
+
+  if (strcmp (kind_string, "push") == 0)
+    {
+      stack = (diagnostic_stack_entry_t *) xmalloc (sizeof (diagnostic_stack_entry_t));
+      stack->previous = diagnostic_stack;
+      stack->state = diagnostic_save_classifications (global_dc);
+      diagnostic_stack = stack;
+ 
+      token = pragma_lex (&x);
+      if (token != CPP_NAME)
+	return;
+      kind_string = IDENTIFIER_POINTER (x);
+    }
+  else if (strcmp (kind_string, "pop") == 0)
+    {
+      stack = diagnostic_stack;
+      if (!stack)
+	{
+	  error ("$pragma GCC diagnostic pop with no matching push");
+	  return;
+	}
+      diagnostic_stack = stack->previous;
+      diagnostic_restore_classifications (global_dc, stack->state);
+      free (stack);
+      return;
+    }
+#if 0
+  /* This is an example of how to do this, in case we decide to need
+     this functionality in the future.  "undo" moves one state up the
+     tree, effectively undoing the previous pragma.  It relies on
+     every pragma being in the state tree (which may be expensive), so
+     we must force the tree to exist somehow.  */
+  else if (strcmp (kind_string, "undo") == 0)
+    {
+      diagnostic_classification_state_t *state;
+      state = diagnostic_save_classifications (global_dc);
+      state = state->previous;
+      diagnostic_restore_classifications (global_dc, state);
+      return;
+    }
+  /* Here we force the state tree to exist for any change-type pragma.  */
+  diagnostic_save_classifications (global_dc);
+#endif
+
   if (strcmp (kind_string, "error") == 0)
     kind = DK_ERROR;
   else if (strcmp (kind_string, "warning") == 0)
     kind = DK_WARNING;
   else if (strcmp (kind_string, "ignored") == 0)
     kind = DK_IGNORED;
   else
-    GCC_BAD ("expected [error|warning|ignored] after %<#pragma GCC diagnostic%>");
+    GCC_BAD ("expected [push|pop|error|warning|ignored] after %<#pragma GCC diagnostic%>");
 
   token = pragma_lex (&x);
   if (token != CPP_STRING)
     GCC_BAD ("missing option after %<#pragma GCC diagnostic%> kind");
   option_string = TREE_STRING_POINTER (x);
   for (option_index = 0; option_index < cl_options_count; option_index++)

Re: Add a __nowarn__ keyword

[Manuel López-Ibáñez]

On 10/08/07, DJ Delorie <dj@redhat.com> wrote:
>
> > | I'm working on it now.
> >
> > awesome!
>
> How's this look?  The classification state change tree is in the
> diagnostic context, but the actual structure is opaque.  The push/pop
> stack is separate, stored in c-pragma.c.  This way, the save/restore
> can be used for scope-level diagnostic state once we figure out how to
> define that, via a (save A, restore B, restore A) for now.  At some
> point, I suppose we could pass the state pointer itself to the
> diagnostic reporting functions.
>

Do we have the scope-level information available when we emit
diagnostics from the middle-end? How do you search for the current
state given a scope? Also, it is not clear to me how the scope of a
pragma is defined by its location. You may have several contradicting
pragmas at file-scope (set to ignore, then restore), how you lookup
the status in that case?

As Gabriel, I am a bit confused by what you want to achieve with the
proposed push/pop. As the way I see it, some user cases could be:

1. Kaveh wants to disable -Wcast-qual for just one macro.

#define WHATEVER \
_Pragma("GCC diagnostics disable -Wcast-qual"); \
my_beautiful_hack \
_Pragma("GCC diagnostics restore -Wcast-qual");

2. DJ wants a header with stricter diagnostics that preserves the
initial state and restores it at the end of the header

#pragma GCC diagnostics error "-Whatever"
#pragma GCC diagnostics error "-Whatever2"
typical things within a header
#pragma GCC diagnostics restore "-Whatever"
#pragma GCC diagnostics restore "-Whatever2"

3. Manuel wants to enable the new -Wconversion that comes with GCC 4.3
but does not want to get weird warnings for older compilers.

#if GCC_VERSION > 40300
#pragma GCC diagnostics enable "-Wconversion"
#else
#pragma GCC diagnostics disable "-Wconversion"
#endif


Yes, a simple
#pragma GCC diagnostics restore
may be shorter for case 2 when you have hundreds of pragmas. It will
mean "undo all changes up to the previous pop/save operation or the
initial state". But I think it will be a bit expensive operation when
the user can simply do copy + paste + s/error/restore/

Maybe (probably) I am misunderstanding your implementation. If you
have some examples as you see it, it will help me to understand how
you envision the feature.

Cheers,

Manuel.

Re: Add a __nowarn__ keyword

[DJ Delorie]

> Do we have the scope-level information available when we emit
> diagnostics from the middle-end?

Not yet.

> How do you search for the current state given a scope?

Yet to be decided.

> Also, it is not clear to me how the scope of a pragma is defined by
> its location.

It's not, you wouldn't use pragmas for scoped changes.

> You may have several contradicting pragmas at file-scope
> (set to ignore, then restore), how you lookup the status in that
> case?

Whatever was in force at that time, which may be undefined for
pragmas.

> As Gabriel, I am a bit confused by what you want to achieve with the
> proposed push/pop.

Adding push/pop was a trivial change to the pragma handler.  It exists
as an example of how to use the save/restore state tree code in
diagnostics.c.  The same save/restore API can be used to keep track of
the diagnostic state for other purposes besides pragmas, like gcc's
__extension__.

> 2. DJ wants a header with stricter diagnostics that preserves the
> initial state and restores it at the end of the header
> 
> #pragma GCC diagnostics error "-Whatever"
> #pragma GCC diagnostics error "-Whatever2"
> typical things within a header
> #pragma GCC diagnostics restore "-Whatever"
> #pragma GCC diagnostics restore "-Whatever2"

That assumes that there's a restore, and that it restores to the right
place (what if other headers have made global changes?)

> Yes, a simple #pragma GCC diagnostics restore may be shorter for
> case 2 when you have hundreds of pragmas. It will mean "undo all
> changes up to the previous pop/save operation or the initial
> state". But I think it will be a bit expensive operation when the
> user can simply do copy + paste + s/error/restore/

Did you look at my patch?  A restore is a simple pointer assignment,
nothing more.

Plus, push/pop can be properly nested, whereas change/restore cannot,
unless you define "restore" as "pop" in which case you might as well
call it that.

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Thu, 9 Aug 2007, Daniel Jacobowitz wrote:

> On Thu, Aug 09, 2007 at 10:19:27AM -0400, Kaveh R. GHAZI wrote:
> > Maybe I don't understand your question, but the keyword vs pragma styles
> > clearly do not work in the same set of cases.  At least, if by "cases" you
> > mean: locations in code where they can be placed and affect.  If by
> > "cases" you mean: which warnings they can effectively silence, that I
> > don't know.  I think both mechanisms have holes in warning coverage that
> > don't necessarily overlap.
> >
> > Which interpretation did you mean?
>
> All of those.  There shouldn't be anywhere you could put __nowarn__
> that you couldn't put a _Pragma, since that is a preprocessing
> directive, not part of the C grammar.  Nor can I see why one would
> work for some warning that the other did not work for.  The natural
> usage style is somewhat different, depending on how the pragma is
> defined... and obviously the implementation would be very different.

_Pragma(diagnostic) is implement to obey the same rules as #pragma
diagnostic with regards to placement.  I.e. if you put a _Pragma anywhere
inside a function definition, you will get an error.

This is a limitation of _Pragma(diagnostic) not _Pragma(something-else).

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Paolo Bonzini]

> _Pragma(diagnostic) is implement to obey the same rules as #pragma
> diagnostic with regards to placement.  I.e. if you put a _Pragma anywhere
> inside a function definition, you will get an error.

Yes, and if we had a "push/pop" mechanism, we could limit 
_Pragma(diagnostic)'s effect to the current expression in the same way 
as __extension__/__nowarn__.

Regarding DJ's worry about unit-at-a-time etc., they obviously cannot 
apply to front-end generated warnings such as these. It is correct that 
GCC's currently implemented approach does not affect reliably warnings 
that are generated by the middle-end, but your __nowarn__ patch wouldn't 
work on them either.

Paolo

Re: Add a __nowarn__ keyword

[Ian Lance Taylor]

"Kaveh R. GHAZI" <ghazi@caip.rutgers.edu> writes:

> One potential problem is if warnings are emitted from elsewhere than in
> the parsing stage.  E.g. if a warning comes from the middle-end, then
> possibly __nowarn__ doesn't get to intercept it using this mechanism.
> I don't know if this occurs in practice, and if so whether it happens
> enough to invalidate this approach.  It sems to work for __extension__ so
> I am cautiously optimistic it'll work for __nowarn__.

We do emit warnings from the middle-end.  Some examples are
-Wunreachable-code, -Wuninitialized, -Wstrict-overflow,
-Wstrict-aliasing=3, -Winline, -Wunsafe-loop-optimizations,
-Warray-bounds.

I think it would be confusing to have __nowarn__ disable some warnings
but not others.  Warnings like -Wuninitialized in particular are ones
which people would naturally expect to be able to disable.

We have a facility to disable warnings in the tree codes: the
TREE_NO_WARNING flag.  It might be possible to use __nowarn__ to set
that flag.  I'm not sure how that would effect the tuples work.

Ian

Re: Add a __nowarn__ keyword

[DJ Delorie]

Ian Lance Taylor <iant@google.com> writes:
> We have a facility to disable warnings in the tree codes: the
> TREE_NO_WARNING flag.

If we add the diagnostic settings chain, what we'd need to do is
somehow refer to a point in the chain from the tree objects, either
directly (a new field - ick) or indirectly (a tree code for "different
warnings for my children").

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

"Kaveh R. GHAZI" <ghazi@caip.rutgers.edu> writes:


[...]

| Still my preference is to go with the original union-hack and avoid this
| altogether. :-)

I have a preference for avoiding the __nowarn__ extension.

-- Gaby

Re: Add a __nowarn__ keyword

[Manuel López-Ibáñez]

On 08/08/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> On Wed, 8 Aug 2007, Paolo Bonzini wrote:
>
> So this is encouraging, I added OPT_Wcast_qual to the warning statement
> and now it obeys the #pragma (or _Pragma).  However I'm still having one
> last problem.  The pragma interface is still on/off, not push/pop.  I.e.:

Can you commit that fix as obvious before we forget about it? Thanks.

> Assuming you've corrected the missing OPT_Wcast_qual in c-typeck.c, if you
> compile the above with -Wcast-qual -Werror, the code correctly avoids the
> warnings on the line marked WARNING1.  However we should still get a
> warning for WARNING2 but we don't.  I believe the pragma has changed the
> behavior of gcc for the rest of the entire file.  There is no "pop" at the
> end of the inline function CONST_CAST.

I don't think there should be any implicit "pop". The pragma is at
file-scope level, so it makes sense it applies to the whole file
unless you provide another pragma later. Even if the pragma were at
function-body scope, I am not sure why you would assume an implicit
"pop" at the end of the function. Making this particular pragmas
context-aware or just location-aware (like #if-#endif) is a matter of
the design we wish to implement. I don't think either of them would be
easier than the other, specially for warnings that don't come from the
front-end.

> In order to see WARNING2, I have to uncomment the second pragma.  But this
> is bad IMHO because it overrides the command line.

Does it change the warning to an error? If that works, we only need a
new keyword "restore" or "default" that changes it to the command-line
value (or default value if there was no command-line setting) and et
voilá, it will work (at least for this case). It is not push/pop but
it is a step forward and I think that would be completely equivalent
to __nowarn__ but more powerful (although arguably, a bit more
verbose, so perhaps __nowarn__ would be still useful). I had a look at
diagnostics.c and I think the implementation would be easy. But
without regular internet connection at home, I cannot really work on
this right now.

Kaveh, DJ, what do you think about this? Am I talking nonsense?

Manuel.

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 544 bytes --]

On Thu, 9 Aug 2007, Manuel López-Ibáñez wrote:

| > In order to see WARNING2, I have to uncomment the second pragma.  But this
| > is bad IMHO because it overrides the command line.
| 
| Does it change the warning to an error? If that works, we only need a
| new keyword "restore" or "default" that changes it to the command-line
| value (or default value if there was no command-line setting) and et

We need "restore" to change the state back its previous value before
the pragma, not just before the setting at the command lines.

-- Gaby

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Thu, 9 Aug 2007, Gabriel Dos Reis wrote:

> We need "restore" to change the state back its previous value before
> the pragma, not just before the setting at the command lines.
> -- Gaby

Agreed.

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Thu, 9 Aug 2007, [ISO-8859-1] Manuel López-Ibáñez wrote:

> On 08/08/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> > On Wed, 8 Aug 2007, Paolo Bonzini wrote:
> >
> > So this is encouraging, I added OPT_Wcast_qual to the warning statement
> > and now it obeys the #pragma (or _Pragma).  However I'm still having one
> > last problem.  The pragma interface is still on/off, not push/pop.  I.e.:
>
> Can you commit that fix as obvious before we forget about it? Thanks.

Yes, I'll do that.  (At the moment, someone killed sparc-solaris
bootstraps.  So even though it's obvious, I'll postpone installing until I
can test it.)

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Thu, 9 Aug 2007, Kaveh R. GHAZI wrote:

> On Thu, 9 Aug 2007, [ISO-8859-1] Manuel López-Ibáñez wrote:
>
> > On 08/08/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> > > On Wed, 8 Aug 2007, Paolo Bonzini wrote:
> > >
> > > So this is encouraging, I added OPT_Wcast_qual to the warning statement
> > > and now it obeys the #pragma (or _Pragma).  However I'm still having one
> > > last problem.  The pragma interface is still on/off, not push/pop.  I.e.:
> >
> > Can you commit that fix as obvious before we forget about it? Thanks.
>
> Yes, I'll do that.  (At the moment, someone killed sparc-solaris
> bootstraps.  So even though it's obvious, I'll postpone installing until I
> can test it.)


Here's the patch, it passes "make cc1".  But as I said can't bootstrap
right now because of this:
http://gcc.gnu.org/ml/gcc/2007-08/msg00149.html

The C++ frontend needs a separate patch, but it's not a one-liner AFAICT.

		--Kaveh


2007-08-09  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

	* c-typeck.c (build_c_cast): Add OPT_Wcast_qual to warnings.

diff -rup orig/egcc-SVN20070808/gcc/c-typeck.c egcc-SVN20070808/gcc/c-typeck.c
--- orig/egcc-SVN20070808/gcc/c-typeck.c	2007-08-08 15:05:41.090014861 -0400
+++ egcc-SVN20070808/gcc/c-typeck.c	2007-08-09 23:04:49.282366645 -0400
@@ -3556,12 +3556,12 @@ build_c_cast (tree type, tree expr)
 		 && TREE_CODE (in_otype) == POINTER_TYPE);

 	  if (added)
-	    warning (0, "cast adds new qualifiers to function type");
+	    warning (OPT_Wcast_qual, "cast adds new qualifiers to function type");

 	  if (discarded)
 	    /* There are qualifiers present in IN_OTYPE that are not
 	       present in IN_TYPE.  */
-	    warning (0, "cast discards qualifiers from pointer target type");
+	    warning (OPT_Wcast_qual, "cast discards qualifiers from pointer target type");
 	}

       /* Warn about possible alignment problems.  */

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1078 bytes --]

On Thu, 9 Aug 2007, Kaveh R. GHAZI wrote:

| On Thu, 9 Aug 2007, Kaveh R. GHAZI wrote:
| 
| > On Thu, 9 Aug 2007, [ISO-8859-1] Manuel López-Ibáñez wrote:
| >
| > > On 08/08/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
| > > > On Wed, 8 Aug 2007, Paolo Bonzini wrote:
| > > >
| > > > So this is encouraging, I added OPT_Wcast_qual to the warning statement
| > > > and now it obeys the #pragma (or _Pragma).  However I'm still having one
| > > > last problem.  The pragma interface is still on/off, not push/pop.  I.e.:
| > >
| > > Can you commit that fix as obvious before we forget about it? Thanks.
| >
| > Yes, I'll do that.  (At the moment, someone killed sparc-solaris
| > bootstraps.  So even though it's obvious, I'll postpone installing until I
| > can test it.)
| 
| 
| Here's the patch, it passes "make cc1".  But as I said can't bootstrap
| right now because of this:
| http://gcc.gnu.org/ml/gcc/2007-08/msg00149.html
| 
| The C++ frontend needs a separate patch, but it's not a one-liner AFAICT.

Patch is OK as soon as bootstrap is green.


Thanks!

-- Gaby

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Wed, 8 Aug 2007, [ISO-8859-1] Manuel López-Ibáñez wrote:

> About the patch itself, I don't understand why you use one approach
> for the C parser and a different one for the C++ parser. I think it
> would be advantageous to use the same approach even if you cannot
> share the relevant functions. I prefer the  cp_parser_nowarn_opt
> approach: the resulting code looks simpler.

In creating the __nowarn__ patch, I copied the mechanism each frontend
used for handling __extension__ and then tweeked it for the new keyword.
Each frontend had it's own way of doing __extension__.  Since that code is
old (i.e. well tested), I don't want to change it gratuitously.


> Also, the patch needs testcases. And a follow-up for GCC 4.3 webpage
> if we want to expose this to users.

I was counting on my CONST_CAST patch to exercise the __nowarn__ keyword,
that way it's tested as part of bootstrapping.  (There's precedence for
not adding testcases for bugs that are exposed through bootstrapping, I
figured feature tests are similar.)  I didn't see any tests for
__extension__ either or I would have copied those.  I can add more tests
as a followup, can you perhaps suggest some?

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[Manuel López-Ibáñez]

On 08/08/2007, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
> > Also, the patch needs testcases. And a follow-up for GCC 4.3 webpage
> > if we want to expose this to users.
>
> I was counting on my CONST_CAST patch to exercise the __nowarn__ keyword,
> that way it's tested as part of bootstrapping.  (There's precedence for
> not adding testcases for bugs that are exposed through bootstrapping, I
> figured feature tests are similar.)  I didn't see any tests for
> __extension__ either or I would have copied those.  I can add more tests
> as a followup, can you perhaps suggest some?
>

I am not a maintainer. If neither the relevant maintainer nor you
consider that testcases are needed, I am OK with that. Just trying to
be constructive.

Cheers,

Manuel.

Re: Add a __nowarn__ keyword

[Paolo Bonzini]

[sorry for breaking the thread]

> +  if (strcmp (kind_string, "push") == 0)
> +    {
> +      stack = (diagnostic_stack_entry_t *) xmalloc (sizeof (diagnostic_stack_entry_t));
> +      stack->previous = diagnostic_stack;
> +      stack->state = diagnostic_save_classifications (global_dc);
> +      diagnostic_stack = stack;
> +      return;
> +    }

> If yes, is the "push" necessary?  Can we make it implicit?

We could have something like this:

     if (memcmp (kind_string, "push", 4) == 0
         && (kind_string[4] == '\0' || ISSPACE (kind_string[4])))
       {
         ...
         diagnostic_stack = stack;
         kind_string += 4;
         while (ISSPACE (*kind_string))
           kind_string++;
         if (*kind_string == '\0')
           return;
       }

which allows both

#pragma GCC diagnostics push

and

#pragma GCC diagnostics push ignore "-Wcast-qual"

Paolo

Re: Add a __nowarn__ keyword

[Gabriel Dos Reis]

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> writes:

| [sorry for breaking the thread]
| 
| > +  if (strcmp (kind_string, "push") == 0)
| > +    {
| > +      stack = (diagnostic_stack_entry_t *) xmalloc (sizeof (diagnostic_stack_entry_t));
| > +      stack->previous = diagnostic_stack;
| > +      stack->state = diagnostic_save_classifications (global_dc);
| > +      diagnostic_stack = stack;
| > +      return;
| > +    }
| 
| > If yes, is the "push" necessary?  Can we make it implicit?
| 
| We could have something like this:
| 
|      if (memcmp (kind_string, "push", 4) == 0
|          && (kind_string[4] == '\0' || ISSPACE (kind_string[4])))
|        {
|          ...
|          diagnostic_stack = stack;
|          kind_string += 4;
|          while (ISSPACE (*kind_string))
|            kind_string++;
|          if (*kind_string == '\0')
|            return;
|        }
| 
| which allows both
| 
| #pragma GCC diagnostics push
| 
| and
| 
| #pragma GCC diagnostics push ignore "-Wcast-qual"


DJ, what do you think?

-- Gaby

Re: Add a __nowarn__ keyword

[DJ Delorie]

Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
> | #pragma GCC diagnostics push ignore "-Wcast-qual"
> 
> DJ, what do you think?

I think that's an easy change.

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Fri, 10 Aug 2007, DJ Delorie wrote:

>
> Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
> > | #pragma GCC diagnostics push ignore "-Wcast-qual"
> >
> > DJ, what do you think?
>
> I think that's an easy change.

DJ, over here:
http://gcc.gnu.org/ml/gcc-patches/2007-08/msg00538.html
you said: "We don't really support changing the diagnostics *after* you
start defining functions.  If that happens to work at all, it's only by
coincidence, especially with file-at-a-time optimizations and tree
inlining."

I expect that for the new push/pop interface to be useful, we'll need to
support multiple uses and allow them to occur midway through the file.

Do you expect this to be supported now?

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu

Re: Add a __nowarn__ keyword

[DJ Delorie]

> DJ, over here:
> http://gcc.gnu.org/ml/gcc-patches/2007-08/msg00538.html you said:
> "We don't really support changing the diagnostics *after* you start
> defining functions.  If that happens to work at all, it's only by
> coincidence, especially with file-at-a-time optimizations and tree
> inlining."

Yes.  That's still the case, because warnings generated after parsing
is complete will always use the state of the pragmas at the end of the
file, not at the point where the function was defined.

Thus, a warning may be generated for line X that doesn't respect what
you'd think the pragmas are set for at line X.

> I expect that for the new push/pop interface to be useful, we'll
> need to support multiple uses and allow them to occur midway through
> the file.

The tricky part is keeping track of *what* state is in effect for a
given function/tree/rtl, so that the right warning state is used.
Additionally, the diagnostics API may need to be updated to pass the
chosen classification state.  Otherwise, we just keep restoring state
as needed.  It could get messy.

Perhaps we could swap state whenever we change cfun ?  Still won't
work for inlined functions, though, as I've said before.

> Do you expect this to be supported now?

Expect?  No.  I'm adding this mechanism because it's easy, and may
*happen* to be useful to wrap functions/headers/whatnot so that at
least parse-time warnings *may* be controlled that way.

Also, it may be the basis for future work that keeps track of the
state more reliably, or for small hacks that need to temporarily
change the warning state for some reason.

I'm certainly not going to volunteer to make sweeping changes to the
MI code to keep track of state for every tree node, rtl, or whatever.

Re: Add a __nowarn__ keyword

[FX Coudert]

Hi Kaveh,

I just want to ask that patches touching Fortran areas (front-end,  
libgfortran, testsuite) be CCed to the fortran mailing-list, which  
allows us to keep track of what is changed before (or when) it  
happens, rather than having to search through mailing-list archives  
when you discover something new in the code (like happened to me  
tonight). Most (all?) if us don't read gcc-patches on a regular basis.

Thanks,
FX

Re: Add a __nowarn__ keyword

[Kaveh R. GHAZI]

On Fri, 10 Aug 2007, FX Coudert wrote:

> Hi Kaveh,
>
> I just want to ask that patches touching Fortran areas (front-end,
> libgfortran, testsuite) be CCed to the fortran mailing-list, which
> allows us to keep track of what is changed before (or when) it
> happens, rather than having to search through mailing-list archives
> when you discover something new in the code (like happened to me
> tonight). Most (all?) if us don't read gcc-patches on a regular basis.
>
> Thanks,
> FX

Ok, sorry I forgot.  For posterity, here's a link to the patch:
http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01993.html
and Mark's approval:
http://gcc.gnu.org/ml/gcc-patches/2007-08/msg00683.html

		--Kaveh
--
Kaveh R. Ghazi			ghazi@caip.rutgers.edu