KDECompilerSettings: Pass -Wvla & -Wdate-time
ClosedPublic

Authored by kfunk on Apr 13 2017, 5:23 PM.

Details

Summary

-Wvla: Warn because it's non-standard feature, not supported by MSVC to date
-Wdate-time: Warn because using TIME or DATE prevents reproducible builds

These warnings are being used for *building* Qt itself as well (cf.
qt_common.prf in qtbase)

Test Plan

No new warnings from rebuilding KDE Frameworks

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kfunk created this revision.Apr 13 2017, 5:23 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptApr 13 2017, 5:23 PM
Restricted Application added subscribers: Build System, Frameworks. · View Herald Transcript
kfunk edited the summary of this revision. (Show Details)Apr 13 2017, 5:24 PM
kfunk edited the test plan for this revision. (Show Details)

For -Wdate-time:
https://lxr.kde.org/search?_filestring=&_string=__DATE__
https://lxr.kde.org/search?_filestring=&_string=__TIME__

-> Only around ~10 locations affected, mainly Krita/Digikam/Amarok stuff. The use of DATE & TIME should just be avoided.

mpyne accepted this revision.Apr 15 2017, 12:41 AM
mpyne added a subscriber: mpyne.

I think these warning flags make sense as default warning flags. I don't agree that __DATE__ and __TIME__ should be avoided as a rule, but projects that find this valuable can disable the warning as appropriate (e.g. with a "developer mode" option) and make alternative provisions for reproducible builds (for packagers or those auditing the binaries generated by others).

kde-modules/KDECompilerSettings.cmake
376

The CMake docs seem to say that the operator precedence of OR vs AND will do what you mean here for the GCC version check. But still, if you're going to defensively parenthesize the Clang version check (and I agree with doing so), you should also do the same for the GCC version check. That way we don't have to check the CMake docs to make sure it's right. :)

This revision is now accepted and ready to land.Apr 15 2017, 12:41 AM
kfunk updated this revision to Diff 13476.Apr 15 2017, 4:58 PM

Address mpyne's concerns

This revision was automatically updated to reflect the committed changes.