| Message ID | 20170524192946.GO12306@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 24, 2017 at 12:29 PM, Jonathan Wakely <jwakely@redhat.com> wrote: > On 24/05/17 14:50 -0400, Jason Merrill wrote: >> >> On Wed, May 24, 2017 at 10:20 AM, Jonathan Wakely <jwakely@redhat.com> >> wrote: >>> >>> On 23/05/17 16:26 -0400, Jason Merrill wrote: >>>> >>>> >>>> On Tue, May 23, 2017 at 2:00 PM, Jonathan Wakely <jwakely@redhat.com> >>>> wrote: >>>>> >>>>> >>>>> On 19/05/17 15:14 -0400, Jason Merrill wrote: >>>>>> >>>>>> >>>>>> >>>>>> On Thu, Apr 27, 2017 at 12:59 PM, Jonathan Wakely <jwakely@redhat.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> I also tried to add a warning like EDG's (see the PR) but it gave a >>>>>>> false positive for direct-list-init of scoped enums (P0138R2, >>>>>>> r240449) >>>>>>> because that code goes through build_c_cast to perform the >>>>>>> conversion, >>>>>>> so looks like a cast to my new warning. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> The enum init code could strip the cv-quals when calling build_c_cast >>>>>> to avoid the warning. >>>>> >>>>> >>>>> >>>>> >>>>> Thanks, that works. I don't think this warning fits under any existing >>>>> option, so I'll add a new one. -Wuseless-cast-qual maybe? Or is that >>>>> too close to -Wuseless-cast and -Wcast-qual and would cause confusion? >>>> >>>> >>>> >>>> -Wcast-rvalue-qual ? >>> >>> >>> >>> I realised that -Wignored-qualifiers is a good fit. That warns about >>> ignored cv-qualifiers on return types, which is a very similar case. >>> >>> This patch adds a new function to conditionally warn and calls that >>> from the build_*_cast functions after they produce a valid result. I >>> originally put the warnings next to the cv_unqualified calls that >>> strip the quals, but was getting duplicate warnings when build_cp_cast >>> calls more than one of the build_*_cast_1 functions. >>> >>> This also removes the unnecessary check for reference types that >>> Nathan pointed out. >>> >>> Tested x86_64-linux and powerpc64-linux. OK for trunk? >> >> >>> +/* >>> + Warns if the cast ignores cv-qualifiers on TYPE. >>> + */ >> >> >> The GCC sources don't put /* and */ on their own line. OK with that >> change, thanks! > > > OK, I'll also fix it on the maybe_warn_about_useless_cast function > just above, which I copied :-) This change caused a bootstrap failure on aarch64-linux-gnu and x86_64-linux-gnu: In file included from ../../gcc/gcc/system.h:691:0, from ../../gcc/gcc/read-rtl.c:31: ../../gcc/gcc/read-rtl.c: In member function ‘const char* md_reader::apply_iterator_to_string(const char*)’: ../../gcc/gcc/../include/libiberty.h:722:38: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers] # define alloca(x) __builtin_alloca(x) ^ ../../gcc/gcc/../include/libiberty.h:727:47: note: in expansion of macro ‘alloca’ char *const libiberty_nptr = (char *const) alloca (libiberty_len); \ ^~~~~~ ../../gcc/gcc/read-rtl.c:380:21: note: in expansion of macro ‘ASTRDUP’ base = p = copy = ASTRDUP (string); ^~~~~~~ I know you did not touch libiberty.h but that is emitting an error. Did you test your patch with a full bootstrap? I thought that was recorded as being required now for C++ patches; I know a few years back when the GCC was not compiling as C++, it was not required. Thanks, Andrew > >
On Wed, May 24, 2017 at 8:07 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Wed, May 24, 2017 at 12:29 PM, Jonathan Wakely <jwakely@redhat.com> wrote: >> On 24/05/17 14:50 -0400, Jason Merrill wrote: >>> >>> On Wed, May 24, 2017 at 10:20 AM, Jonathan Wakely <jwakely@redhat.com> >>> wrote: >>>> >>>> On 23/05/17 16:26 -0400, Jason Merrill wrote: >>>>> >>>>> >>>>> On Tue, May 23, 2017 at 2:00 PM, Jonathan Wakely <jwakely@redhat.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> On 19/05/17 15:14 -0400, Jason Merrill wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Thu, Apr 27, 2017 at 12:59 PM, Jonathan Wakely <jwakely@redhat.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> I also tried to add a warning like EDG's (see the PR) but it gave a >>>>>>>> false positive for direct-list-init of scoped enums (P0138R2, >>>>>>>> r240449) >>>>>>>> because that code goes through build_c_cast to perform the >>>>>>>> conversion, >>>>>>>> so looks like a cast to my new warning. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> The enum init code could strip the cv-quals when calling build_c_cast >>>>>>> to avoid the warning. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Thanks, that works. I don't think this warning fits under any existing >>>>>> option, so I'll add a new one. -Wuseless-cast-qual maybe? Or is that >>>>>> too close to -Wuseless-cast and -Wcast-qual and would cause confusion? >>>>> >>>>> >>>>> >>>>> -Wcast-rvalue-qual ? >>>> >>>> >>>> >>>> I realised that -Wignored-qualifiers is a good fit. That warns about >>>> ignored cv-qualifiers on return types, which is a very similar case. >>>> >>>> This patch adds a new function to conditionally warn and calls that >>>> from the build_*_cast functions after they produce a valid result. I >>>> originally put the warnings next to the cv_unqualified calls that >>>> strip the quals, but was getting duplicate warnings when build_cp_cast >>>> calls more than one of the build_*_cast_1 functions. >>>> >>>> This also removes the unnecessary check for reference types that >>>> Nathan pointed out. >>>> >>>> Tested x86_64-linux and powerpc64-linux. OK for trunk? >>> >>> >>>> +/* >>>> + Warns if the cast ignores cv-qualifiers on TYPE. >>>> + */ >>> >>> >>> The GCC sources don't put /* and */ on their own line. OK with that >>> change, thanks! >> >> >> OK, I'll also fix it on the maybe_warn_about_useless_cast function >> just above, which I copied :-) > > This change caused a bootstrap failure on aarch64-linux-gnu and > x86_64-linux-gnu: > In file included from ../../gcc/gcc/system.h:691:0, > from ../../gcc/gcc/read-rtl.c:31: > ../../gcc/gcc/read-rtl.c: In member function ‘const char* > md_reader::apply_iterator_to_string(const char*)’: > ../../gcc/gcc/../include/libiberty.h:722:38: error: type qualifiers > ignored on cast result type [-Werror=ignored-qualifiers] > # define alloca(x) __builtin_alloca(x) > ^ > ../../gcc/gcc/../include/libiberty.h:727:47: note: in expansion of > macro ‘alloca’ > char *const libiberty_nptr = (char *const) alloca (libiberty_len); \ > ^~~~~~ > ../../gcc/gcc/read-rtl.c:380:21: note: in expansion of macro ‘ASTRDUP’ > base = p = copy = ASTRDUP (string); > ^~~~~~~ > > I know you did not touch libiberty.h but that is emitting an error. > Did you test your patch with a full bootstrap? I thought that was > recorded as being required now for C++ patches; I know a few years > back when the GCC was not compiling as C++, it was not required. Oh it looks like it was already fixed by revision 248442. Just my build automated build was not done for that timeframe. Thanks, Andrew > > Thanks, > Andrew > > >> >>
On 24/05/17 20:09 -0700, Andrew Pinski wrote: >On Wed, May 24, 2017 at 8:07 PM, Andrew Pinski <pinskia@gmail.com> wrote: >> This change caused a bootstrap failure on aarch64-linux-gnu and >> x86_64-linux-gnu: >> In file included from ../../gcc/gcc/system.h:691:0, >> from ../../gcc/gcc/read-rtl.c:31: >> ../../gcc/gcc/read-rtl.c: In member function ‘const char* >> md_reader::apply_iterator_to_string(const char*)’: >> ../../gcc/gcc/../include/libiberty.h:722:38: error: type qualifiers >> ignored on cast result type [-Werror=ignored-qualifiers] >> # define alloca(x) __builtin_alloca(x) >> ^ >> ../../gcc/gcc/../include/libiberty.h:727:47: note: in expansion of >> macro ‘alloca’ >> char *const libiberty_nptr = (char *const) alloca (libiberty_len); \ >> ^~~~~~ >> ../../gcc/gcc/read-rtl.c:380:21: note: in expansion of macro ‘ASTRDUP’ >> base = p = copy = ASTRDUP (string); >> ^~~~~~~ >> >> I know you did not touch libiberty.h but that is emitting an error. >> Did you test your patch with a full bootstrap? I thought that was >> recorded as being required now for C++ patches; I know a few years >> back when the GCC was not compiling as C++, it was not required. > >Oh it looks like it was already fixed by revision 248442. Just my >build automated build was not done for that timeframe. I thought I did, but a --disable-bootstrap slipped in there, copy&pasted from another build, sorry.
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index ecc7190..b81d6c8 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -6655,9 +6655,7 @@ check_for_casting_away_constness (tree src_type, tree dest_type, } } -/* - Warns if the cast from expression EXPR to type TYPE is useless. - */ +/* Warns if the cast from expression EXPR to type TYPE is useless. */ void maybe_warn_about_useless_cast (tree type, tree expr, tsubst_flags_t complain) { @@ -6673,9 +6671,7 @@ maybe_warn_about_useless_cast (tree type, tree expr, tsubst_flags_t complain) } } -/* - Warns if the cast ignores cv-qualifiers on TYPE. - */ +/* Warns if the cast ignores cv-qualifiers on TYPE. */ void maybe_warn_about_cast_ignoring_quals (tree type, tsubst_flags_t complain) {