On Thu, 2021-05-27 at 10:41 -0600, Martin Sebor wrote: > On 5/27/21 5:19 AM, Richard Sandiford wrote: > > Thanks for doing this. > > > > Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > > […] > > > On 5/24/21 5:08 PM, David Malcolm wrote: > > > > On Mon, 2021-05-24 at 16:02 -0600, Martin Sebor wrote: > > > > > Subsequent patches then replace invocations of the > > > > > TREE_NO_WARNING() > > > > > macro and the gimple_no_warning_p() and > > > > > gimple_set_no_warning() > > > > > functions throughout GCC with those and remove the legacy > > > > > APIs to > > > > > keep them from being accidentally reintroduced along with the > > > > > problem. > > > > > These are mostly mechanical changes, except that most of the > > > > > new > > > > > invocations also specify the option whose disposition to > > > > > query for > > > > > the expression or location, or which to enable or disable[2]. > > > > > The last function, copy_no_warning(), copies the disposition > > > > > from > > > > > one expression or location to another. > > > > > > > > > > A couple of design choices might be helpful to explain: > > > > > > > > > > First, introducing "warning groups" rather than controlling > > > > > each > > > > > individual warning is motivated by a) the fact that the > > > > > latter > > > > > would make avoiding redundant warnings for related problems > > > > > cumbersome (e.g., after issuing a -Warray-bounds we want to > > > > > suppress -Wstringop-overflow as well -Wstringop-overread for > > > > > the same access and vice versa), and b) simplicity and > > > > > efficiency > > > > > of the implementation (mapping each option would require a > > > > > more > > > > > complex data structure like a bitmap). > > > > > > > > > > Second, using location_t to associate expressions/statements > > > > > with > > > > > the warning groups also turns out to be more useful in > > > > > practice > > > > > than a direct association between a tree or gimple*, and also > > > > > simplifies managing the data structure. Trees and gimple* > > > > > come > > > > > and go across passes, and maintaining a mapping for them that > > > > > accounts for the possibility of them being garbage-collected > > > > > and the same addresses reused is less than straightforward. > > > > > > > > I find some of the terminology rather awkard due to it the > > > > negation > > > > we're already using with TREE_NO_WARNING, in that we're turning > > > > on a > > > > no_warning flag, and that this is a per-location/expr/stmt > > > > thing that's > > > > different from the idea of enabling/disabling a specific > > > > warning > > > > altogether (and the pragmas that control that). Sometimes the > > > > patches > > > > refer to enabling/disabling warnings and I think I want > > > > "enabling" and > > > > "disabling" to mean the thing the user does with -Wfoo and - > > > > Wno-foo. > > > > > > > > Would calling it a "warning suppression" or somesuch be more or > > > > less > > > > klunky? > > > > > > I like warning suppression :) But I'm not sure where you suggest > > > I use the phrase. > > > > > > I don't particularly care for the "no" in the API names either > > > (existing or new) and would prefer a positive form. I considered > > > enable_warning() and warning_enabled() but I chose the names I > > > did > > > because they're close to their established gimple namesakes. I'm > > > fine changing them to the alternatives, or if you or someone else > > > has a preference for different names I'm open to suggestions. > > > Let > > > me know. > > > > Not my area, but FWIW, +1 for David's suggestion of > > s/set_no_warning/suppress_warning/ or similar. As well as the > > problem with the double negative in negated conditions, I always > > have to > > remember whether TREE_NO_WARNING means that hasn't been anything to > > warn > > about yet or whether we shouldn't warn in future. > > Sure. Jason has a patch out for review to introduce > > warning_enabled_at (location, int option).
CCing Jason. BTW, do you have a URL for that patch? > > With that, how does the following API look? (I'd replace the int > argument above with opt_code myself): > > void enable_warning (location_t, enum opt_code = N_OPTS); > void disable_warning (location_t, enum opt_code = N_OPTS); > void copy_warning (location_t, location_t); > bool warning_enabled_at (location_t, enum opt_code = N_OPTS). > > void enable_warning (tree, enum opt_code = N_OPTS); > void disable_warning (tree, enum opt_code = N_OPTS); > void copy_warning (tree, const_tree); > bool warning_enabled_at (const_tree, enum opt_code = N_OPTS). > > void enable_warning (gimple *, enum opt_code = N_OPTS); > void disable_warning (gimple *, enum opt_code = N_OPTS); > void copy_warning (gimple *, const gimple *); > bool warning_enabled_at (gimple *, enum opt_code = N_OPTS). I'm not so happy with "enable_warning" and "disable_warning" in the above proposal; it seems to me that three things are being conflated: (a) whether the user enabled the warning with -Wfoo, whether at the command line, or by #pragma (and using the location_t to query the pragma information) versus (b) whether we're suppressing a category of warnings for this construct (and using the location_t to stash the flags) versus (c) should the warning actually be emitted at the location Presumably Jason's patch is a way for our warning code (in the FE and middle end) to avoid doing expensive things by querying whether the warning would actually get emitted i.e. it's a way to query (c), so we can have an early-reject if it returns false, where presumably the query is cheap relative to the testing that would otherwise occur. Is that right? Perhaps the terminology should be that: - the user *requests* a warning, (a) above - our code can *suppress* a warning, for (b) above - the combination of the two give (c): whether the warning is *enabled* Or maybe (a) the user *enables* a warning (b) our code can *suppress* a warning (or warning category) (c) the combination gives whether the warning is *active*. or somesuch. Thoughts? Dave
