| Message ID | 87zjlydltr.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On 2/11/2014 4:45 AM, Richard Sandiford wrote: > OK, this version drops the "[enabled by default]" altogether. > Tested as before. OK to install? Still a huge earthquake in terms of affecting test suites and baselines of many users. is it really worth it? In the case of GNAT we have only recently started tagging messages in this way, so changes would not be so disruptive, and we can debate following whatever gcc does, but I think it is important to understand that any change in this area is a big one in terms of impact on users. > > Thanks, > Richard > > > gcc/ > * opts.c (option_name): Remove "enabled by default" rider. > > gcc/testsuite/ > * gcc.dg/gomp/simd-clones-5.c: Update comment for new warning message. > > Index: gcc/opts.c > =================================================================== > --- gcc/opts.c 2014-02-10 20:36:32.380197329 +0000 > +++ gcc/opts.c 2014-02-10 20:58:45.894502379 +0000 > @@ -2216,14 +2216,10 @@ option_name (diagnostic_context *context > return xstrdup (cl_options[option_index].opt_text); > } > /* A warning without option classified as an error. */ > - else if (orig_diag_kind == DK_WARNING || orig_diag_kind == DK_PEDWARN > - || diag_kind == DK_WARNING) > - { > - if (context->warning_as_error_requested) > - return xstrdup (cl_options[OPT_Werror].opt_text); > - else > - return xstrdup (_("enabled by default")); > - } > + else if ((orig_diag_kind == DK_WARNING || orig_diag_kind == DK_PEDWARN > + || diag_kind == DK_WARNING) > + && context->warning_as_error_requested) > + return xstrdup (cl_options[OPT_Werror].opt_text); > else > return NULL; > } > Index: gcc/testsuite/gcc.dg/gomp/simd-clones-5.c > =================================================================== > --- gcc/testsuite/gcc.dg/gomp/simd-clones-5.c 2014-02-10 20:36:32.380197329 +0000 > +++ gcc/testsuite/gcc.dg/gomp/simd-clones-5.c 2014-02-10 21:00:32.549412313 +0000 > @@ -3,7 +3,7 @@ > > /* ?? The -w above is to inhibit the following warning for now: > a.c:2:6: warning: AVX vector argument without AVX enabled changes > - the ABI [enabled by default]. */ > + the ABI. */ > > #pragma omp declare simd notinbranch simdlen(4) > void foo (int *a) >
Robert Dewar <dewar@adacore.com> writes: > On 2/11/2014 4:45 AM, Richard Sandiford wrote: >> OK, this version drops the "[enabled by default]" altogether. >> Tested as before. OK to install? > > Still a huge earthquake in terms of affecting test suites and > baselines of many users. is it really worth it? In the case of > GNAT we have only recently started tagging messages in this > way, so changes would not be so disruptive, and we can debate > following whatever gcc does, but I think it is important to > understand that any change in this area is a big one in terms > of impact on users. The patch deliberately didn't affect Ada's diagnostic routines given your comments from the first round. Calling this a "huge earthquake" for other languages seems like a gross overstatement. I don't think gcc, g++, gfortran, etc, have ever made a commitment to producing textually identical warnings and errors for given inputs across different releases. It seems ridiculous to require that, especially if it stands in the way of improving the diagnostics or introducing finer-grained -W control. E.g. Florian's complaint was that we shouldn't have warnings that are not under the control of any -W options. But by your logic we couldn't change that either, because all those "[enabled by default]"s would become "[-Wnew-option]"s. Thanks, Richard
On Tue, Feb 11, 2014 at 1:48 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Robert Dewar <dewar@adacore.com> writes: >> On 2/11/2014 4:45 AM, Richard Sandiford wrote: >>> OK, this version drops the "[enabled by default]" altogether. >>> Tested as before. OK to install? >> >> Still a huge earthquake in terms of affecting test suites and >> baselines of many users. is it really worth it? In the case of >> GNAT we have only recently started tagging messages in this >> way, so changes would not be so disruptive, and we can debate >> following whatever gcc does, but I think it is important to >> understand that any change in this area is a big one in terms >> of impact on users. > > The patch deliberately didn't affect Ada's diagnostic routines given > your comments from the first round. Calling this a "huge earthquake" > for other languages seems like a gross overstatement. > > I don't think gcc, g++, gfortran, etc, have ever made a commitment > to producing textually identical warnings and errors for given inputs > across different releases. It seems ridiculous to require that, > especially if it stands in the way of improving the diagnostics > or introducing finer-grained -W control. > > E.g. Florian's complaint was that we shouldn't have warnings that > are not under the control of any -W options. But by your logic > we couldn't change that either, because all those "[enabled by default]"s > would become "[-Wnew-option]"s. Yeah, I think Roberts argument is a red herring - there are loads of diagnostic changes every release so you cannot expect those to be stable. I'm fine with dropping the [enabled by default] as in the patch, but lets hear another "ok" before making the change. Thanks, Richard. > Thanks, > Richard
On 2/11/2014 7:48 AM, Richard Sandiford wrote: > The patch deliberately didn't affect Ada's diagnostic routines given > your comments from the first round. Calling this a "huge earthquake" > for other languages seems like a gross overstatement. Actually it's much less of an impact for Ada for two reasons. First we only just started tagging warnings. In fact we have only just released an official version with the facility for tagging warnings. Second, this tagging of warnings is not the default (that would have been a big earthquake) but you have to turn it on explicitly. But I do indeed think it will have a significant impact for users of other languages, where this has been done for a while, and if I am not mistaken, done by default? > I don't think gcc, g++, gfortran, etc, have ever made a commitment > to producing textually identical warnings and errors for given inputs > across different releases. It seems ridiculous to require that, > especially if it stands in the way of improving the diagnostics > or introducing finer-grained -W control. > > E.g. Florian's complaint was that we shouldn't have warnings that > are not under the control of any -W options. But by your logic > we couldn't change that either, because all those "[enabled by default]"s > would become "[-Wnew-option]"s. > I am not saying you can't change it, just that it is indeed a big earthquake. No of course there is no commitment not to make changes. But you have to be aware that when you make changes like this, the impact is very significant in real production environments, and gcc is as you know extensively used in such environments. What I am saying here is that this is worth some discussion on what the best approach is. Ideally indeed it would be better if all warnings were controlled by some specific warning category. I am not sure a warning switch that default-covered all otherwise uncovered cases (as suggested by one person at least) would be a worthwhile approach.
Robert Dewar <dewar@adacore.com> writes: >> I don't think gcc, g++, gfortran, etc, have ever made a commitment >> to producing textually identical warnings and errors for given inputs >> across different releases. It seems ridiculous to require that, >> especially if it stands in the way of improving the diagnostics >> or introducing finer-grained -W control. >> >> E.g. Florian's complaint was that we shouldn't have warnings that >> are not under the control of any -W options. But by your logic >> we couldn't change that either, because all those "[enabled by default]"s >> would become "[-Wnew-option]"s. >> > I am not saying you can't change it, just that it is indeed a big > earthquake. No of course there is no commitment not to make changes. > But you have to be aware that when you make changes like this, the > impact is very significant in real production environments, and > gcc is as you know extensively used in such environments. > > What I am saying here is that this is worth some discussion on what > the best approach is. But what's the basis for that discussion going to be? I first made this suggestion on gcc@, which is the best list we have for getting user feedback, and no user made this objection. And when I worked in an environment where I had direct contact with GCC-using customers, none of them gave any indication that they were expecting textual stability between releases. If you know of people who are using non-Ada languages this way then please describe their set-up. If you don't, how are we going to know how such hypothetical users are going to react? E.g. how many of those users will have heard of "sed"? I thought the trend these days was to move towards -Werror, so that for many people the expected output is to get no warnings at all. And bear in mind that the kind of warnings that are not under -W control tend to be those that are so likely to be a mistake that no-one has ever had an incentive to make them optional. I find it hard to believe that significant numbers of users are not fixing the sources of those warnings and are instead requiring every release of GCC to produce warnings with a particular wording. Thanks, Richard
On 2/11/2014 9:36 AM, Richard Sandiford wrote: > I find it hard to believe that > significant numbers of users are not fixing the sources of those > warnings and are instead requiring every release of GCC to produce > warnings with a particular wording. Good enough for me, I think it is OK to make the change.
Am 2014-02-11 15:36, schrieb Richard Sandiford: > I thought the trend these days was to move towards -Werror, so that for > many people the expected output is to get no warnings at all. And bear > in mind that the kind of warnings that are not under -W control tend to > be those that are so likely to be a mistake that no-one has ever had an > incentive to make them optional. I find it hard to believe that > significant numbers of users are not fixing the sources of those > warnings and are instead requiring every release of GCC to produce > warnings with a particular wording. Hi, actually at my site we turn on more and more warnings into errors, but we do it warning by warning with more -Werror=..., so the fine-grained warning changes are really nice for us. The problem we face with "[enabled by default]" warnings is not that there are no options to turn these warnings off (we _want_ these warnings), but this also means there are no corresponding -Werror= options (and also no -Werror=enabled-by-default or -Werror=default-warnings). And pure -Werror turns all other warnings we want to see into errors too :(. regards, Franz
On Tue, Feb 11, 2014 at 5:20 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Tue, Feb 11, 2014 at 1:48 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Robert Dewar <dewar@adacore.com> writes: >>> On 2/11/2014 4:45 AM, Richard Sandiford wrote: >>>> OK, this version drops the "[enabled by default]" altogether. >>>> Tested as before. OK to install? >>> >>> Still a huge earthquake in terms of affecting test suites and >>> baselines of many users. is it really worth it? In the case of >>> GNAT we have only recently started tagging messages in this >>> way, so changes would not be so disruptive, and we can debate >>> following whatever gcc does, but I think it is important to >>> understand that any change in this area is a big one in terms >>> of impact on users. >> >> The patch deliberately didn't affect Ada's diagnostic routines given >> your comments from the first round. Calling this a "huge earthquake" >> for other languages seems like a gross overstatement. >> >> I don't think gcc, g++, gfortran, etc, have ever made a commitment >> to producing textually identical warnings and errors for given inputs >> across different releases. It seems ridiculous to require that, >> especially if it stands in the way of improving the diagnostics >> or introducing finer-grained -W control. >> >> E.g. Florian's complaint was that we shouldn't have warnings that >> are not under the control of any -W options. But by your logic >> we couldn't change that either, because all those "[enabled by default]"s >> would become "[-Wnew-option]"s. > > Yeah, I think Roberts argument is a red herring - there are loads of > diagnostic changes every release so you cannot expect those to > be stable. > > I'm fine with dropping the [enabled by default] as in the patch, but lets > hear another "ok" before making the change. I think this change is OK. It's obviously not a great situation, but "enabled by default" is fairly useless information, so this seems like a marginal improvement. Ian
Index: gcc/opts.c =================================================================== --- gcc/opts.c 2014-02-10 20:36:32.380197329 +0000 +++ gcc/opts.c 2014-02-10 20:58:45.894502379 +0000 @@ -2216,14 +2216,10 @@ option_name (diagnostic_context *context return xstrdup (cl_options[option_index].opt_text); } /* A warning without option classified as an error. */ - else if (orig_diag_kind == DK_WARNING || orig_diag_kind == DK_PEDWARN - || diag_kind == DK_WARNING) - { - if (context->warning_as_error_requested) - return xstrdup (cl_options[OPT_Werror].opt_text); - else - return xstrdup (_("enabled by default")); - } + else if ((orig_diag_kind == DK_WARNING || orig_diag_kind == DK_PEDWARN + || diag_kind == DK_WARNING) + && context->warning_as_error_requested) + return xstrdup (cl_options[OPT_Werror].opt_text); else return NULL; } Index: gcc/testsuite/gcc.dg/gomp/simd-clones-5.c =================================================================== --- gcc/testsuite/gcc.dg/gomp/simd-clones-5.c 2014-02-10 20:36:32.380197329 +0000 +++ gcc/testsuite/gcc.dg/gomp/simd-clones-5.c 2014-02-10 21:00:32.549412313 +0000 @@ -3,7 +3,7 @@ /* ?? The -w above is to inhibit the following warning for now: a.c:2:6: warning: AVX vector argument without AVX enabled changes - the ABI [enabled by default]. */ + the ABI. */ #pragma omp declare simd notinbranch simdlen(4) void foo (int *a)