* [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro @ 2007-07-27 8:15 Kaveh R. GHAZI 2007-07-27 9:53 ` Richard Guenther 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-07-27 8:15 UTC (permalink / raw) To: gcc-patches 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; } ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-07-27 8:15 [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro Kaveh R. GHAZI @ 2007-07-27 9:53 ` Richard Guenther 2007-07-27 17:24 ` Kaveh R. GHAZI 0 siblings, 1 reply; 79+ messages in thread From: Richard Guenther @ 2007-07-27 9:53 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-07-27 9:53 ` Richard Guenther @ 2007-07-27 17:24 ` Kaveh R. GHAZI 2007-07-27 19:32 ` Richard Guenther 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-07-27 17:24 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches 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; } ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-07-27 17:24 ` Kaveh R. GHAZI @ 2007-07-27 19:32 ` Richard Guenther 2007-07-27 19:37 ` Kaveh R. GHAZI 0 siblings, 1 reply; 79+ messages in thread From: Richard Guenther @ 2007-07-27 19:32 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-07-27 19:32 ` Richard Guenther @ 2007-07-27 19:37 ` Kaveh R. GHAZI 2007-07-27 20:28 ` Gabriel Dos Reis 2007-08-03 14:19 ` Kaveh R. GHAZI 0 siblings, 2 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-07-27 19:37 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-07-27 19:37 ` Kaveh R. GHAZI @ 2007-07-27 20:28 ` Gabriel Dos Reis 2007-08-03 14:19 ` Kaveh R. GHAZI 1 sibling, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-07-27 20:28 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Richard Guenther, gcc-patches "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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-07-27 19:37 ` Kaveh R. GHAZI 2007-07-27 20:28 ` Gabriel Dos Reis @ 2007-08-03 14:19 ` Kaveh R. GHAZI 2007-08-03 18:07 ` Gabriel Dos Reis 1 sibling, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-03 14:19 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-03 14:19 ` Kaveh R. GHAZI @ 2007-08-03 18:07 ` Gabriel Dos Reis 2007-08-06 0:19 ` Mark Mitchell 0 siblings, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-03 18:07 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Richard Guenther, gcc-patches "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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-03 18:07 ` Gabriel Dos Reis @ 2007-08-06 0:19 ` Mark Mitchell 2007-08-06 0:32 ` Gabriel Dos Reis 0 siblings, 1 reply; 79+ messages in thread From: Mark Mitchell @ 2007-08-06 0:19 UTC (permalink / raw) To: Gabriel Dos Reis; +Cc: Kaveh R. GHAZI, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 0:19 ` Mark Mitchell @ 2007-08-06 0:32 ` Gabriel Dos Reis 2007-08-06 4:42 ` Kaveh R. GHAZI 0 siblings, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-06 0:32 UTC (permalink / raw) To: Mark Mitchell; +Cc: Kaveh R. GHAZI, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 0:32 ` Gabriel Dos Reis @ 2007-08-06 4:42 ` Kaveh R. GHAZI 2007-08-06 5:23 ` Mark Mitchell 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-06 4:42 UTC (permalink / raw) To: Gabriel Dos Reis; +Cc: Mark Mitchell, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 4:42 ` Kaveh R. GHAZI @ 2007-08-06 5:23 ` Mark Mitchell 2007-08-06 14:09 ` Kaveh R. GHAZI 0 siblings, 1 reply; 79+ messages in thread From: Mark Mitchell @ 2007-08-06 5:23 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Gabriel Dos Reis, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 5:23 ` Mark Mitchell @ 2007-08-06 14:09 ` Kaveh R. GHAZI 2007-08-06 15:33 ` Gabriel Dos Reis 2007-08-06 15:44 ` Mark Mitchell 0 siblings, 2 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-06 14:09 UTC (permalink / raw) To: Mark Mitchell; +Cc: Gabriel Dos Reis, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 14:09 ` Kaveh R. GHAZI @ 2007-08-06 15:33 ` Gabriel Dos Reis 2007-08-06 17:59 ` Kaveh R. GHAZI 2007-08-06 15:44 ` Mark Mitchell 1 sibling, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-06 15:33 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Mark Mitchell, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 15:33 ` Gabriel Dos Reis @ 2007-08-06 17:59 ` Kaveh R. GHAZI 2007-08-06 18:11 ` DJ Delorie 2007-08-06 18:18 ` Gabriel Dos Reis 0 siblings, 2 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-06 17:59 UTC (permalink / raw) To: Gabriel Dos Reis; +Cc: Mark Mitchell, Richard Guenther, gcc-patches, dj 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 17:59 ` Kaveh R. GHAZI @ 2007-08-06 18:11 ` DJ Delorie 2007-08-06 18:23 ` Gabriel Dos Reis 2007-08-06 18:18 ` Gabriel Dos Reis 1 sibling, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-06 18:11 UTC (permalink / raw) To: ghazi; +Cc: gdr, mark, richard.guenther, gcc-patches > 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 18:11 ` DJ Delorie @ 2007-08-06 18:23 ` Gabriel Dos Reis 0 siblings, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-06 18:23 UTC (permalink / raw) To: DJ Delorie; +Cc: ghazi, mark, richard.guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 17:59 ` Kaveh R. GHAZI 2007-08-06 18:11 ` DJ Delorie @ 2007-08-06 18:18 ` Gabriel Dos Reis 1 sibling, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-06 18:18 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Mark Mitchell, Richard Guenther, gcc-patches, dj 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro 2007-08-06 14:09 ` Kaveh R. GHAZI 2007-08-06 15:33 ` Gabriel Dos Reis @ 2007-08-06 15:44 ` Mark Mitchell 2007-08-08 5:04 ` Add a __nowarn__ keyword Kaveh R. GHAZI 1 sibling, 1 reply; 79+ messages in thread From: Mark Mitchell @ 2007-08-06 15:44 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Gabriel Dos Reis, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Add a __nowarn__ keyword 2007-08-06 15:44 ` Mark Mitchell @ 2007-08-08 5:04 ` Kaveh R. GHAZI 2007-08-08 8:52 ` Manuel López-Ibáñez 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-08 5:04 UTC (permalink / raw) To: Mark Mitchell; +Cc: Gabriel Dos Reis, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 5:04 ` Add a __nowarn__ keyword Kaveh R. GHAZI @ 2007-08-08 8:52 ` Manuel López-Ibáñez 2007-08-08 9:04 ` Gabriel Dos Reis 2007-08-08 12:56 ` Kaveh R. GHAZI 0 siblings, 2 replies; 79+ messages in thread From: Manuel López-Ibáñez @ 2007-08-08 8:52 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Mark Mitchell, Gabriel Dos Reis, Richard Guenther, gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 8:52 ` Manuel López-Ibáñez @ 2007-08-08 9:04 ` Gabriel Dos Reis 2007-08-08 13:06 ` Kaveh R. GHAZI 2007-08-08 12:56 ` Kaveh R. GHAZI 1 sibling, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-08 9:04 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Kaveh R. GHAZI, Mark Mitchell, Richard Guenther, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 9:04 ` Gabriel Dos Reis @ 2007-08-08 13:06 ` Kaveh R. GHAZI 2007-08-08 13:16 ` Gabriel Dos Reis 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-08 13:06 UTC (permalink / raw) To: Gabriel Dos Reis Cc: Manuel López-Ibáñez, Mark Mitchell, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 13:06 ` Kaveh R. GHAZI @ 2007-08-08 13:16 ` Gabriel Dos Reis 2007-08-08 13:48 ` Kaveh R. GHAZI 0 siblings, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-08 13:16 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Manuel López-Ibáñez, Mark Mitchell, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 13:16 ` Gabriel Dos Reis @ 2007-08-08 13:48 ` Kaveh R. GHAZI 2007-08-08 13:58 ` Paolo Bonzini 2007-08-08 14:29 ` Manuel López-Ibáñez 0 siblings, 2 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-08 13:48 UTC (permalink / raw) To: Gabriel Dos Reis Cc: Manuel López-Ibáñez, Mark Mitchell, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 13:48 ` Kaveh R. GHAZI @ 2007-08-08 13:58 ` Paolo Bonzini 2007-08-10 1:42 ` Gabriel Dos Reis 2007-08-08 14:29 ` Manuel López-Ibáñez 1 sibling, 1 reply; 79+ messages in thread From: Paolo Bonzini @ 2007-08-08 13:58 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Gabriel Dos Reis, Manuel López-Ibáñez, Mark Mitchell, gcc-patches > 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 13:58 ` Paolo Bonzini @ 2007-08-10 1:42 ` Gabriel Dos Reis 0 siblings, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 1:42 UTC (permalink / raw) To: Paolo Bonzini Cc: Kaveh R. GHAZI, Manuel López-Ibáñez, Mark Mitchell, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 13:48 ` Kaveh R. GHAZI 2007-08-08 13:58 ` Paolo Bonzini @ 2007-08-08 14:29 ` Manuel López-Ibáñez 2007-08-08 15:25 ` Daniel Jacobowitz 2007-08-08 16:35 ` Paolo Bonzini 1 sibling, 2 replies; 79+ messages in thread From: Manuel López-Ibáñez @ 2007-08-08 14:29 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Gabriel Dos Reis, Mark Mitchell, Richard Guenther, gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 14:29 ` Manuel López-Ibáñez @ 2007-08-08 15:25 ` Daniel Jacobowitz 2007-08-08 16:35 ` Paolo Bonzini 1 sibling, 0 replies; 79+ messages in thread From: Daniel Jacobowitz @ 2007-08-08 15:25 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Kaveh R. GHAZI, Gabriel Dos Reis, Mark Mitchell, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 14:29 ` Manuel López-Ibáñez 2007-08-08 15:25 ` Daniel Jacobowitz @ 2007-08-08 16:35 ` Paolo Bonzini 2007-08-08 19:31 ` Kaveh R. GHAZI 1 sibling, 1 reply; 79+ messages in thread From: Paolo Bonzini @ 2007-08-08 16:35 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Kaveh R. GHAZI, Gabriel Dos Reis, Mark Mitchell, gcc-patches > 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 16:35 ` Paolo Bonzini @ 2007-08-08 19:31 ` Kaveh R. GHAZI 2007-08-08 19:42 ` Gabriel Dos Reis ` (2 more replies) 0 siblings, 3 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-08 19:31 UTC (permalink / raw) To: Paolo Bonzini Cc: Manuel López-Ibáñez, Gabriel Dos Reis, Mark Mitchell, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 19:31 ` Kaveh R. GHAZI @ 2007-08-08 19:42 ` Gabriel Dos Reis 2007-08-08 20:22 ` Paolo Bonzini 2007-08-08 19:51 ` DJ Delorie 2007-08-09 14:41 ` Manuel López-Ibáñez 2 siblings, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-08 19:42 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Paolo Bonzini, Manuel López-Ibáñez, Mark Mitchell, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 19:42 ` Gabriel Dos Reis @ 2007-08-08 20:22 ` Paolo Bonzini 0 siblings, 0 replies; 79+ messages in thread From: Paolo Bonzini @ 2007-08-08 20:22 UTC (permalink / raw) To: Gabriel Dos Reis Cc: Kaveh R. GHAZI, Paolo Bonzini, Manuel López-Ibáñez, Mark Mitchell, gcc-patches > 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 19:31 ` Kaveh R. GHAZI 2007-08-08 19:42 ` Gabriel Dos Reis @ 2007-08-08 19:51 ` DJ Delorie 2007-08-08 22:41 ` Kaveh R. GHAZI 2007-08-09 14:41 ` Manuel López-Ibáñez 2 siblings, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-08 19:51 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: gcc-patches "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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 19:51 ` DJ Delorie @ 2007-08-08 22:41 ` Kaveh R. GHAZI 2007-08-08 22:54 ` DJ Delorie 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-08 22:41 UTC (permalink / raw) To: DJ Delorie; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 22:41 ` Kaveh R. GHAZI @ 2007-08-08 22:54 ` DJ Delorie 2007-08-09 2:36 ` Kaveh R. GHAZI 0 siblings, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-08 22:54 UTC (permalink / raw) To: ghazi; +Cc: gcc-patches > 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? ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 22:54 ` DJ Delorie @ 2007-08-09 2:36 ` Kaveh R. GHAZI 2007-08-09 13:40 ` Daniel Jacobowitz ` (2 more replies) 0 siblings, 3 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-09 2:36 UTC (permalink / raw) To: DJ Delorie; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 2:36 ` Kaveh R. GHAZI @ 2007-08-09 13:40 ` Daniel Jacobowitz 2007-08-09 14:19 ` Kaveh R. GHAZI 2007-08-09 20:04 ` Ian Lance Taylor 2007-08-10 1:47 ` Gabriel Dos Reis 2 siblings, 1 reply; 79+ messages in thread From: Daniel Jacobowitz @ 2007-08-09 13:40 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: DJ Delorie, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 13:40 ` Daniel Jacobowitz @ 2007-08-09 14:19 ` Kaveh R. GHAZI 2007-08-09 14:30 ` Daniel Jacobowitz 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-09 14:19 UTC (permalink / raw) To: Daniel Jacobowitz; +Cc: DJ Delorie, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 14:19 ` Kaveh R. GHAZI @ 2007-08-09 14:30 ` Daniel Jacobowitz 2007-08-09 15:05 ` Manuel López-Ibáñez 2007-08-09 15:21 ` Kaveh R. GHAZI 0 siblings, 2 replies; 79+ messages in thread From: Daniel Jacobowitz @ 2007-08-09 14:30 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: DJ Delorie, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 14:30 ` Daniel Jacobowitz @ 2007-08-09 15:05 ` Manuel López-Ibáñez 2007-08-09 15:15 ` Daniel Jacobowitz 2007-08-09 15:31 ` DJ Delorie 2007-08-09 15:21 ` Kaveh R. GHAZI 1 sibling, 2 replies; 79+ messages in thread From: Manuel López-Ibáñez @ 2007-08-09 15:05 UTC (permalink / raw) To: Kaveh R. GHAZI, DJ Delorie, gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 15:05 ` Manuel López-Ibáñez @ 2007-08-09 15:15 ` Daniel Jacobowitz 2007-08-09 15:31 ` DJ Delorie 1 sibling, 0 replies; 79+ messages in thread From: Daniel Jacobowitz @ 2007-08-09 15:15 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 15:05 ` Manuel López-Ibáñez 2007-08-09 15:15 ` Daniel Jacobowitz @ 2007-08-09 15:31 ` DJ Delorie 2007-08-09 22:23 ` Mark Mitchell 1 sibling, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-09 15:31 UTC (permalink / raw) To: lopezibanez; +Cc: ghazi, gcc-patches > 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 15:31 ` DJ Delorie @ 2007-08-09 22:23 ` Mark Mitchell 2007-08-09 22:43 ` Kaveh R. GHAZI 2007-08-10 1:50 ` Gabriel Dos Reis 0 siblings, 2 replies; 79+ messages in thread From: Mark Mitchell @ 2007-08-09 22:23 UTC (permalink / raw) To: DJ Delorie; +Cc: lopezibanez, ghazi, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 22:23 ` Mark Mitchell @ 2007-08-09 22:43 ` Kaveh R. GHAZI 2007-08-10 1:52 ` Gabriel Dos Reis 2007-08-10 16:42 ` Mark Mitchell 2007-08-10 1:50 ` Gabriel Dos Reis 1 sibling, 2 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-09 22:43 UTC (permalink / raw) To: Mark Mitchell; +Cc: DJ Delorie, lopezibanez, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 22:43 ` Kaveh R. GHAZI @ 2007-08-10 1:52 ` Gabriel Dos Reis 2007-08-10 16:42 ` Mark Mitchell 1 sibling, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 1:52 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Mark Mitchell, DJ Delorie, lopezibanez, gcc-patches "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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 22:43 ` Kaveh R. GHAZI 2007-08-10 1:52 ` Gabriel Dos Reis @ 2007-08-10 16:42 ` Mark Mitchell 1 sibling, 0 replies; 79+ messages in thread From: Mark Mitchell @ 2007-08-10 16:42 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: DJ Delorie, lopezibanez, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 22:23 ` Mark Mitchell 2007-08-09 22:43 ` Kaveh R. GHAZI @ 2007-08-10 1:50 ` Gabriel Dos Reis 2007-08-10 2:02 ` DJ Delorie 1 sibling, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 1:50 UTC (permalink / raw) To: Mark Mitchell; +Cc: DJ Delorie, lopezibanez, ghazi, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 1:50 ` Gabriel Dos Reis @ 2007-08-10 2:02 ` DJ Delorie 2007-08-10 3:09 ` Gabriel Dos Reis 0 siblings, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-10 2:02 UTC (permalink / raw) To: gdr; +Cc: mark, lopezibanez, ghazi, gcc-patches > I fully agree with that. I'm working on it now. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 2:02 ` DJ Delorie @ 2007-08-10 3:09 ` Gabriel Dos Reis 2007-08-10 3:28 ` DJ Delorie 0 siblings, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 3:09 UTC (permalink / raw) To: DJ Delorie; +Cc: mark, lopezibanez, ghazi, gcc-patches On Thu, 9 Aug 2007, DJ Delorie wrote: | | > I fully agree with that. | | I'm working on it now. awesome! -- Gaby ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 3:09 ` Gabriel Dos Reis @ 2007-08-10 3:28 ` DJ Delorie 2007-08-10 3:44 ` Gabriel Dos Reis 2007-08-10 10:04 ` Manuel López-Ibáñez 0 siblings, 2 replies; 79+ messages in thread From: DJ Delorie @ 2007-08-10 3:28 UTC (permalink / raw) To: gdr; +Cc: mark, lopezibanez, ghazi, gcc-patches > | 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; ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 3:28 ` DJ Delorie @ 2007-08-10 3:44 ` Gabriel Dos Reis 2007-08-10 4:00 ` DJ Delorie 2007-08-10 10:04 ` Manuel López-Ibáñez 1 sibling, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 3:44 UTC (permalink / raw) To: DJ Delorie; +Cc: mark, lopezibanez, ghazi, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 3:44 ` Gabriel Dos Reis @ 2007-08-10 4:00 ` DJ Delorie 2007-08-10 4:12 ` Gabriel Dos Reis 0 siblings, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-10 4:00 UTC (permalink / raw) To: gdr; +Cc: mark, lopezibanez, ghazi, gcc-patches > 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? ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 4:00 ` DJ Delorie @ 2007-08-10 4:12 ` Gabriel Dos Reis 2007-08-10 4:23 ` DJ Delorie ` (2 more replies) 0 siblings, 3 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 4:12 UTC (permalink / raw) To: DJ Delorie; +Cc: mark, lopezibanez, ghazi, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 4:12 ` Gabriel Dos Reis @ 2007-08-10 4:23 ` DJ Delorie 2007-08-10 13:24 ` Gabriel Dos Reis 2007-08-10 18:40 ` DJ Delorie 2007-08-10 19:05 ` DJ Delorie 2 siblings, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-10 4:23 UTC (permalink / raw) To: gdr; +Cc: mark, lopezibanez, ghazi, gcc-patches > 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 4:23 ` DJ Delorie @ 2007-08-10 13:24 ` Gabriel Dos Reis 0 siblings, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 13:24 UTC (permalink / raw) To: DJ Delorie; +Cc: mark, lopezibanez, ghazi, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 4:12 ` Gabriel Dos Reis 2007-08-10 4:23 ` DJ Delorie @ 2007-08-10 18:40 ` DJ Delorie 2007-08-11 19:19 ` Joseph S. Myers 2007-08-10 19:05 ` DJ Delorie 2 siblings, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-10 18:40 UTC (permalink / raw) To: gdr; +Cc: mark, lopezibanez, ghazi, gcc-patches 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_*. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 18:40 ` DJ Delorie @ 2007-08-11 19:19 ` Joseph S. Myers 2007-08-13 18:36 ` DJ Delorie 2007-08-21 3:24 ` DJ Delorie 0 siblings, 2 replies; 79+ messages in thread From: Joseph S. Myers @ 2007-08-11 19:19 UTC (permalink / raw) To: DJ Delorie; +Cc: gdr, Mark Mitchell, lopezibanez, ghazi, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-11 19:19 ` Joseph S. Myers @ 2007-08-13 18:36 ` DJ Delorie 2007-08-21 3:24 ` DJ Delorie 1 sibling, 0 replies; 79+ messages in thread From: DJ Delorie @ 2007-08-13 18:36 UTC (permalink / raw) To: joseph; +Cc: gdr, mark, lopezibanez, ghazi, gcc-patches > 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-11 19:19 ` Joseph S. Myers 2007-08-13 18:36 ` DJ Delorie @ 2007-08-21 3:24 ` DJ Delorie 2009-11-21 12:24 ` Magnus Fromreide 1 sibling, 1 reply; 79+ messages in thread From: DJ Delorie @ 2007-08-21 3:24 UTC (permalink / raw) To: joseph; +Cc: gdr, mark, lopezibanez, ghazi, gcc-patches 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. */ ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-21 3:24 ` DJ Delorie @ 2009-11-21 12:24 ` Magnus Fromreide 2009-11-23 16:40 ` Manuel López-Ibáñez 2009-11-23 23:28 ` DJ Delorie 0 siblings, 2 replies; 79+ messages in thread From: Magnus Fromreide @ 2009-11-21 12:24 UTC (permalink / raw) To: DJ Delorie; +Cc: joseph, gdr, mark, lopezibanez, ghazi, gcc-patches 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) ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2009-11-21 12:24 ` Magnus Fromreide @ 2009-11-23 16:40 ` Manuel López-Ibáñez 2009-11-23 23:28 ` DJ Delorie 1 sibling, 0 replies; 79+ messages in thread From: Manuel López-Ibáñez @ 2009-11-23 16:40 UTC (permalink / raw) To: Magnus Fromreide; +Cc: DJ Delorie, joseph, gdr, mark, ghazi, gcc-patches 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) > > ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2009-11-21 12:24 ` Magnus Fromreide 2009-11-23 16:40 ` Manuel López-Ibáñez @ 2009-11-23 23:28 ` DJ Delorie 1 sibling, 0 replies; 79+ messages in thread From: DJ Delorie @ 2009-11-23 23:28 UTC (permalink / raw) To: Magnus Fromreide; +Cc: joseph, gdr, mark, lopezibanez, ghazi, gcc-patches > 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 4:12 ` Gabriel Dos Reis 2007-08-10 4:23 ` DJ Delorie 2007-08-10 18:40 ` DJ Delorie @ 2007-08-10 19:05 ` DJ Delorie 2 siblings, 0 replies; 79+ messages in thread From: DJ Delorie @ 2007-08-10 19:05 UTC (permalink / raw) To: gdr; +Cc: mark, lopezibanez, ghazi, gcc-patches 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++) ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 3:28 ` DJ Delorie 2007-08-10 3:44 ` Gabriel Dos Reis @ 2007-08-10 10:04 ` Manuel López-Ibáñez 2007-08-10 18:46 ` DJ Delorie 1 sibling, 1 reply; 79+ messages in thread From: Manuel López-Ibáñez @ 2007-08-10 10:04 UTC (permalink / raw) To: DJ Delorie; +Cc: gdr, mark, ghazi, gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 10:04 ` Manuel López-Ibáñez @ 2007-08-10 18:46 ` DJ Delorie 0 siblings, 0 replies; 79+ messages in thread From: DJ Delorie @ 2007-08-10 18:46 UTC (permalink / raw) To: lopezibanez; +Cc: gdr, mark, ghazi, gcc-patches > 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 14:30 ` Daniel Jacobowitz 2007-08-09 15:05 ` Manuel López-Ibáñez @ 2007-08-09 15:21 ` Kaveh R. GHAZI 2007-08-09 16:48 ` Paolo Bonzini 1 sibling, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-09 15:21 UTC (permalink / raw) To: Daniel Jacobowitz; +Cc: DJ Delorie, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 15:21 ` Kaveh R. GHAZI @ 2007-08-09 16:48 ` Paolo Bonzini 0 siblings, 0 replies; 79+ messages in thread From: Paolo Bonzini @ 2007-08-09 16:48 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: Daniel Jacobowitz, DJ Delorie, gcc-patches > _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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 2:36 ` Kaveh R. GHAZI 2007-08-09 13:40 ` Daniel Jacobowitz @ 2007-08-09 20:04 ` Ian Lance Taylor 2007-08-09 20:40 ` DJ Delorie 2007-08-10 1:47 ` Gabriel Dos Reis 2 siblings, 1 reply; 79+ messages in thread From: Ian Lance Taylor @ 2007-08-09 20:04 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: gcc-patches "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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 20:04 ` Ian Lance Taylor @ 2007-08-09 20:40 ` DJ Delorie 0 siblings, 0 replies; 79+ messages in thread From: DJ Delorie @ 2007-08-09 20:40 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: gcc-patches 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"). ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 2:36 ` Kaveh R. GHAZI 2007-08-09 13:40 ` Daniel Jacobowitz 2007-08-09 20:04 ` Ian Lance Taylor @ 2007-08-10 1:47 ` Gabriel Dos Reis 2 siblings, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 1:47 UTC (permalink / raw) To: Kaveh R. GHAZI; +Cc: DJ Delorie, gcc-patches "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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 19:31 ` Kaveh R. GHAZI 2007-08-08 19:42 ` Gabriel Dos Reis 2007-08-08 19:51 ` DJ Delorie @ 2007-08-09 14:41 ` Manuel López-Ibáñez 2007-08-09 16:36 ` Gabriel Dos Reis 2007-08-09 22:55 ` Kaveh R. GHAZI 2 siblings, 2 replies; 79+ messages in thread From: Manuel López-Ibáñez @ 2007-08-09 14:41 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Paolo Bonzini, Gabriel Dos Reis, Mark Mitchell, gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 14:41 ` Manuel López-Ibáñez @ 2007-08-09 16:36 ` Gabriel Dos Reis 2007-08-09 23:14 ` Kaveh R. GHAZI 2007-08-09 22:55 ` Kaveh R. GHAZI 1 sibling, 1 reply; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-09 16:36 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Kaveh R. GHAZI, Paolo Bonzini, Mark Mitchell, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 16:36 ` Gabriel Dos Reis @ 2007-08-09 23:14 ` Kaveh R. GHAZI 0 siblings, 0 replies; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-09 23:14 UTC (permalink / raw) To: Gabriel Dos Reis Cc: Manuel López-Ibáñez, Paolo Bonzini, Mark Mitchell, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 14:41 ` Manuel López-Ibáñez 2007-08-09 16:36 ` Gabriel Dos Reis @ 2007-08-09 22:55 ` Kaveh R. GHAZI 2007-08-10 3:18 ` Kaveh R. GHAZI 1 sibling, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-09 22:55 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Paolo Bonzini, Gabriel Dos Reis, Mark Mitchell, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-09 22:55 ` Kaveh R. GHAZI @ 2007-08-10 3:18 ` Kaveh R. GHAZI 2007-08-10 3:25 ` Gabriel Dos Reis 0 siblings, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-10 3:18 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Paolo Bonzini, Gabriel Dos Reis, Mark Mitchell, gcc-patches, dj 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. */ ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-10 3:18 ` Kaveh R. GHAZI @ 2007-08-10 3:25 ` Gabriel Dos Reis 0 siblings, 0 replies; 79+ messages in thread From: Gabriel Dos Reis @ 2007-08-10 3:25 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Manuel López-Ibáñez, Paolo Bonzini, Mark Mitchell, gcc-patches, dj [-- 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 8:52 ` Manuel López-Ibáñez 2007-08-08 9:04 ` Gabriel Dos Reis @ 2007-08-08 12:56 ` Kaveh R. GHAZI 2007-08-08 14:05 ` Manuel López-Ibáñez 1 sibling, 1 reply; 79+ messages in thread From: Kaveh R. GHAZI @ 2007-08-08 12:56 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Mark Mitchell, Gabriel Dos Reis, Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 79+ messages in thread
* Re: Add a __nowarn__ keyword 2007-08-08 12:56 ` Kaveh R. GHAZI @ 2007-08-08 14:05 ` Manuel López-Ibáñez 0 siblings, 0 replies; 79+ messages in thread From: Manuel López-Ibáñez @ 2007-08-08 14:05 UTC (permalink / raw) To: Kaveh R. GHAZI Cc: Mark Mitchell, Gabriel Dos Reis, Richard Guenther, gcc-patches 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. ^ permalink raw reply [flat|nested] 79+ messages in thread
end of thread, other threads:[~2009-11-23 23:27 UTC | newest] Thread overview: 79+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-07-27 8:15 [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro Kaveh R. GHAZI 2007-07-27 9:53 ` Richard Guenther 2007-07-27 17:24 ` Kaveh R. GHAZI 2007-07-27 19:32 ` Richard Guenther 2007-07-27 19:37 ` Kaveh R. GHAZI 2007-07-27 20:28 ` Gabriel Dos Reis 2007-08-03 14:19 ` Kaveh R. GHAZI 2007-08-03 18:07 ` Gabriel Dos Reis 2007-08-06 0:19 ` Mark Mitchell 2007-08-06 0:32 ` Gabriel Dos Reis 2007-08-06 4:42 ` Kaveh R. GHAZI 2007-08-06 5:23 ` Mark Mitchell 2007-08-06 14:09 ` Kaveh R. GHAZI 2007-08-06 15:33 ` Gabriel Dos Reis 2007-08-06 17:59 ` Kaveh R. GHAZI 2007-08-06 18:11 ` DJ Delorie 2007-08-06 18:23 ` Gabriel Dos Reis 2007-08-06 18:18 ` Gabriel Dos Reis 2007-08-06 15:44 ` Mark Mitchell 2007-08-08 5:04 ` Add a __nowarn__ keyword Kaveh R. GHAZI 2007-08-08 8:52 ` Manuel López-Ibáñez 2007-08-08 9:04 ` Gabriel Dos Reis 2007-08-08 13:06 ` Kaveh R. GHAZI 2007-08-08 13:16 ` Gabriel Dos Reis 2007-08-08 13:48 ` Kaveh R. GHAZI 2007-08-08 13:58 ` Paolo Bonzini 2007-08-10 1:42 ` Gabriel Dos Reis 2007-08-08 14:29 ` Manuel López-Ibáñez 2007-08-08 15:25 ` Daniel Jacobowitz 2007-08-08 16:35 ` Paolo Bonzini 2007-08-08 19:31 ` Kaveh R. GHAZI 2007-08-08 19:42 ` Gabriel Dos Reis 2007-08-08 20:22 ` Paolo Bonzini 2007-08-08 19:51 ` DJ Delorie 2007-08-08 22:41 ` Kaveh R. GHAZI 2007-08-08 22:54 ` DJ Delorie 2007-08-09 2:36 ` Kaveh R. GHAZI 2007-08-09 13:40 ` Daniel Jacobowitz 2007-08-09 14:19 ` Kaveh R. GHAZI 2007-08-09 14:30 ` Daniel Jacobowitz 2007-08-09 15:05 ` Manuel López-Ibáñez 2007-08-09 15:15 ` Daniel Jacobowitz 2007-08-09 15:31 ` DJ Delorie 2007-08-09 22:23 ` Mark Mitchell 2007-08-09 22:43 ` Kaveh R. GHAZI 2007-08-10 1:52 ` Gabriel Dos Reis 2007-08-10 16:42 ` Mark Mitchell 2007-08-10 1:50 ` Gabriel Dos Reis 2007-08-10 2:02 ` DJ Delorie 2007-08-10 3:09 ` Gabriel Dos Reis 2007-08-10 3:28 ` DJ Delorie 2007-08-10 3:44 ` Gabriel Dos Reis 2007-08-10 4:00 ` DJ Delorie 2007-08-10 4:12 ` Gabriel Dos Reis 2007-08-10 4:23 ` DJ Delorie 2007-08-10 13:24 ` Gabriel Dos Reis 2007-08-10 18:40 ` DJ Delorie 2007-08-11 19:19 ` Joseph S. Myers 2007-08-13 18:36 ` DJ Delorie 2007-08-21 3:24 ` DJ Delorie 2009-11-21 12:24 ` Magnus Fromreide 2009-11-23 16:40 ` Manuel López-Ibáñez 2009-11-23 23:28 ` DJ Delorie 2007-08-10 19:05 ` DJ Delorie 2007-08-10 10:04 ` Manuel López-Ibáñez 2007-08-10 18:46 ` DJ Delorie 2007-08-09 15:21 ` Kaveh R. GHAZI 2007-08-09 16:48 ` Paolo Bonzini 2007-08-09 20:04 ` Ian Lance Taylor 2007-08-09 20:40 ` DJ Delorie 2007-08-10 1:47 ` Gabriel Dos Reis 2007-08-09 14:41 ` Manuel López-Ibáñez 2007-08-09 16:36 ` Gabriel Dos Reis 2007-08-09 23:14 ` Kaveh R. GHAZI 2007-08-09 22:55 ` Kaveh R. GHAZI 2007-08-10 3:18 ` Kaveh R. GHAZI 2007-08-10 3:25 ` Gabriel Dos Reis 2007-08-08 12:56 ` Kaveh R. GHAZI 2007-08-08 14:05 ` Manuel López-Ibáñez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).