Optimize #include
ClosedPublic

Authored by rominf on Mar 6 2018, 8:00 AM.

Details

Reviewers
rkflx
Group Reviewers
Spectacle
Commits
R166:3b69ac3a7aff: Optimize #include
Test Plan

If it compiles, all is fine.

Diff Detail

Repository
R166 Spectacle
Branch
unused-include
Lint
No Linters Available
Unit
No Unit Test Coverage
rominf requested review of this revision.Mar 6 2018, 8:00 AM
rominf created this revision.
rkflx requested changes to this revision.Mar 6 2018, 3:59 PM
rkflx added a subscriber: rkflx.

Thanks for the patch!

If it compiles, all is fine.

That's not enough ;) Changing the include order could also have side effects on the behaviour. Nevertheless, as far as I can see all is fine.

Clang and GCC (4.8 and 7) are happy, and basic functions seemed to work.


Please keep this.

Let me explain: config.h defines various constants which are injected by CMake. You are removing all occurrences but one, exploiting transitivity in the includes. However, that is very dangerous: As soon as this include chain breaks, e.g. if someone moves some includes around in an unrelated patch, the constants won't be defined anymore in that particular compilation unit, without any warning or error.

To prevent this mistake from happening, it's important to directly include config.h everywhere such constants are used.

src/Gui/ExportMenu.cpp
35

Please keep this.

src/Gui/KSMainWindow.cpp
21

Please keep this.

src/Main.cpp
35

Please keep this.

src/PlatformBackends/KWinWaylandImageGrabber.cpp
30

This should be kept and changed to <cerrno>.

src/SpectacleCore.cpp
38

Please keep this.

This revision now requires changes to proceed.Mar 6 2018, 3:59 PM
rominf updated this revision to Diff 28861.Mar 6 2018, 7:04 PM
rominf marked 5 inline comments as done.

Revert removing "Config.h"

rkflx accepted this revision.Mar 6 2018, 8:31 PM
This revision is now accepted and ready to land.Mar 6 2018, 8:31 PM
rominf updated this revision to Diff 28882.Mar 7 2018, 6:55 AM

Rebase on master

This revision was automatically updated to reflect the committed changes.
broulik added a subscriber: broulik.Mar 7 2018, 8:43 AM

This supposedly breaks build https://bugs.kde.org/show_bug.cgi?id=391504 (built fine here, though, perhaps it only breaks if you do not have kipi or Purpose)

rkflx added a comment.Mar 7 2018, 8:43 AM

This supposedly breaks build https://bugs.kde.org/show_bug.cgi?id=391504 (built fine here, though, perhaps it only breaks if you do not have kipi or Purpose)

Thanks for noticing, we are actually already working on it in 3b69ac3a7aff.