| Message ID | 20151121200735.GA55464@troutmask.apl.washington.edu |
|---|---|
| State | New |
| Headers | show |
> + * simplify.c (gfc_simplify_cshift): Work around bootstrap issues > + due to inappropriate warning options. The warning options are appropriate, any dead code can potentially hide a bug and should be flagged; every static analyzer will also do it and we would soon have PRs opened with bugzilla if we tolerated it.
On Sat, Nov 21, 2015 at 11:26:17PM +0100, Eric Botcazou wrote: > > + * simplify.c (gfc_simplify_cshift): Work around bootstrap issues > > + due to inappropriate warning options. > > The warning options are appropriate, any dead code can potentially hide a bug > and should be flagged; every static analyzer will also do it and we would soon > have PRs opened with bugzilla if we tolerated it. > I disgree. If bootstrap is going to enforce -Werror -Wunused-*, then this should be the default for any possible invocation of make.
On Sat, 21 Nov 2015, Steve Kargl wrote: > 2015-11-21 Steven G. Kargl <kargl@gcc.gnu.org> > > * simplify.c (gfc_simplify_cshift): Work around bootstrap issues > due to inappropriate warning options. > Index: simplify.c > =================================================================== > + /* GCC bootstrap is too stupid to realize that the above code for dm > + is correct. First, dim can be specified for a rank 1 array. It is > + not needed in this nor used here. Second, the code is simply waiting > + for someone to implement rank > 1 simplification. For now, add a > + pessimization to the code that has a zero valid reason to be here. */ > + if (dm > array->rank) > + gcc_unreachable (); I'm not sure this comment is appropriate as is. At a minimum, it should list the version of GCC this was introduced for/with. So, something like "As of GCC 6, when bootstrapping we do not realize that..." Gerald
On Thu, Dec 31, 2015 at 09:57:10PM +0800, Gerald Pfeifer wrote: > On Sat, 21 Nov 2015, Steve Kargl wrote: > > 2015-11-21 Steven G. Kargl <kargl@gcc.gnu.org> > > > > * simplify.c (gfc_simplify_cshift): Work around bootstrap issues > > due to inappropriate warning options. > > > Index: simplify.c > > =================================================================== > > + /* GCC bootstrap is too stupid to realize that the above code for dm > > + is correct. First, dim can be specified for a rank 1 array. It is > > + not needed in this nor used here. Second, the code is simply waiting > > + for someone to implement rank > 1 simplification. For now, add a > > + pessimization to the code that has a zero valid reason to be here. */ > > + if (dm > array->rank) > > + gcc_unreachable (); > > I'm not sure this comment is appropriate as is. At a minimum, it > should list the version of GCC this was introduced for/with. So, > something like > I have no intention to change the comment.
Index: ChangeLog =================================================================== --- ChangeLog (revision 230709) +++ ChangeLog (working copy) @@ -1,5 +1,10 @@ 2015-11-21 Steven G. Kargl <kargl@gcc.gnu.org> + * simplify.c (gfc_simplify_cshift): Work around bootstrap issues + due to inappropriate warning options. + +2015-11-21 Steven G. Kargl <kargl@gcc.gnu.org> + * simplify.c (gfc_simplify_cshift): Implement simplification of CSHIFT for rank=1 arrays. (gfc_simplify_spread): Remove a FIXME and add error condition. Index: simplify.c =================================================================== --- simplify.c (revision 230709) +++ simplify.c (working copy) @@ -1869,6 +1869,15 @@ gfc_simplify_cshift (gfc_expr *array, gf else { /* FIXME: Deal with rank > 1 arrays. For now, don't leak memory. */ + + /* GCC bootstrap is too stupid to realize that the above code for dm + is correct. First, dim can be specified for a rank 1 array. It is + not needed in this nor used here. Second, the code is simply waiting + for someone to implement rank > 1 simplification. For now, add a + pessimization to the code that has a zero valid reason to be here. */ + if (dm > array->rank) + gcc_unreachable (); + gfc_free_expr (a); }