Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/ --- (Updated Jan. 23, 2017, 7:04 p.m.) Status -- This change has been marked as submitted. Review request for Build System, KDE Frameworks and Stephen Kelly. Changes --- Submitted with commit faeb56f8137d738d1dfbc1fcfbbacd1e8caeb6f0 by Albert Astals Cid to branch master. Repository: extra-cmake-modules Description --- Gives a nice warning about something that should be marked as override but isn't Diffs - kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 Diff: https://git.reviewboard.kde.org/r/129724/diff/ Testing --- Thanks, Albert Astals Cid
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
> On Jan. 16, 2017, 8:53 a.m., Kevin Funk wrote: > > @Stephen: Let's re-introduce this patch? Laurent + me have applied > > `Q_DECL_OVERRIDE` everywhere. Do you want to double-check? Sorry, I didn't get pinged by email about this, despite the mention from Albert below. Re-applying this sounds fine to me. Thanks for taking care of the porting! - Stephen --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review102057 --- On Jan. 16, 2017, 9:12 a.m., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Jan. 16, 2017, 9:12 a.m.) > > > Review request for Build System, KDE Frameworks and Stephen Kelly. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review102057 --- @Stephen: Let's re-introduce this patch? Laurent + me have applied `Q_DECL_OVERRIDE` everywhere. Do you want to double-check? - Kevin Funk On Dec. 29, 2016, 11:48 p.m., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Dec. 29, 2016, 11:48 p.m.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review101822 --- Note: This got reverted in master for now, since there were too many warnings caused by this change. We'll reapply it after the KF5 release. Most of the warnings have been fixed by Laurent a few days ago (and I have a few more patches I'll push after the release). - Kevin Funk On Dec. 29, 2016, 11:48 p.m., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Dec. 29, 2016, 11:48 p.m.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
> On Dec. 30, 2016, 6:39 a.m., Martin Gräßlin wrote: > > Is that enabled by default now? I hope not! This is a completely useless > > warning for all frameworks (as we are not allowed to use override) and even > > more for a legacy code bases. I don't want to have to adjust the cmake in > > all projects I maintain to silence this warning again. And even less I want > > to spent days adding overrides to legacy code base. > > Laurent Montel wrote: > We can use Q_DECL_OVERRIDE which is replaced by override when your gcc > support it. So There is not a problem to use this flags no ? > > Martin Gräßlin wrote: > I commented on that aspect in the past. We cannot have both: enforce > C++11 and at the same time keep compatibility to no C++11. > > > > We need to find a real line and not bullshit around with macros. > > > > Either we say C++11 then enable all of it, or say no. But then no > earnings please. > > > > I'm seriously annoyed by the stupid dance we are doing. The Clang warning flag is not documented very well but everything I've been able to find seems to indicate it became a default warning when it was added in LLVM 3.6. However some simple testcases I've run against a more recent LLVM (3.9) fail to trip the warning even after enabling optimization, ensuring C++11 is enabled, using various combinations of override combinations, etc. So it seems that at least in recent LLVM this may not be a source of much noise even though it's enabled by default. But there are a lot of complaints online about this warning for LLVM 3.6 so it's going to be with us one way or another anyways. I'm sympathetic to the point about either supporting C++11 or not instead of having to guess which of its subfeatures we can use, especially since our "supported compilers page" (https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11) that tells us what we can use appears to be resistant to being located from a search engine. But that's the kind of thing that would need discussion on the mailing list and it seems to me like we've repainted that shed several times over already. - Michael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review101664 --- On Dec. 29, 2016, 11:48 p.m., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Dec. 29, 2016, 11:48 p.m.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
> On Dez. 30, 2016, 7:39 vorm., Martin Gräßlin wrote: > > Is that enabled by default now? I hope not! This is a completely useless > > warning for all frameworks (as we are not allowed to use override) and even > > more for a legacy code bases. I don't want to have to adjust the cmake in > > all projects I maintain to silence this warning again. And even less I want > > to spent days adding overrides to legacy code base. > > Laurent Montel wrote: > We can use Q_DECL_OVERRIDE which is replaced by override when your gcc > support it. So There is not a problem to use this flags no ? I commented on that aspect in the past. We cannot have both: enforce C++11 and at the same time keep compatibility to no C++11. We need to find a real line and not bullshit around with macros. Either we say C++11 then enable all of it, or say no. But then no earnings please. I'm seriously annoyed by the stupid dance we are doing. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review101664 --- On Dez. 30, 2016, 12:48 vorm., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Dez. 30, 2016, 12:48 vorm.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
> On Dec. 30, 2016, 6:39 a.m., Martin Gräßlin wrote: > > Is that enabled by default now? I hope not! This is a completely useless > > warning for all frameworks (as we are not allowed to use override) and even > > more for a legacy code bases. I don't want to have to adjust the cmake in > > all projects I maintain to silence this warning again. And even less I want > > to spent days adding overrides to legacy code base. We can use Q_DECL_OVERRIDE which is replaced by override when your gcc support it. So There is not a problem to use this flags no ? - Laurent --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review101664 --- On Dec. 29, 2016, 11:48 p.m., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Dec. 29, 2016, 11:48 p.m.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review101664 --- Is that enabled by default now? I hope not! This is a completely useless warning for all frameworks (as we are not allowed to use override) and even more for a legacy code bases. I don't want to have to adjust the cmake in all projects I maintain to silence this warning again. And even less I want to spent days adding overrides to legacy code base. - Martin Gräßlin On Dec. 30, 2016, 12:48 a.m., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Dec. 30, 2016, 12:48 a.m.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/ --- (Updated Dec. 29, 2016, 11:48 p.m.) Status -- This change has been marked as submitted. Review request for Build System and KDE Frameworks. Changes --- Submitted with commit d1d637fadd6dad68995d44101250ebbc3307ed0b by Albert Astals Cid to branch master. Repository: extra-cmake-modules Description --- Gives a nice warning about something that should be marked as override but isn't Diffs - kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 Diff: https://git.reviewboard.kde.org/r/129724/diff/ Testing --- Thanks, Albert Astals Cid
Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/#review101662 --- Ship it! LGTM, and the Clang equivalent (-Winconsistent-missing-override) appears to already be picked up in its -Wall setting. - Michael Pyne On Dec. 29, 2016, 11:07 p.m., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129724/ > --- > > (Updated Dec. 29, 2016, 11:07 p.m.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > --- > > Gives a nice warning about something that should be marked as override but > isn't > > > Diffs > - > > kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 > > Diff: https://git.reviewboard.kde.org/r/129724/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129724/ --- Review request for Build System and KDE Frameworks. Repository: extra-cmake-modules Description --- Gives a nice warning about something that should be marked as override but isn't Diffs - kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 Diff: https://git.reviewboard.kde.org/r/129724/diff/ Testing --- Thanks, Albert Astals Cid
