Page MenuHomeWildfire Games

Delete GUI.h umbrella include
ClosedPublic

Authored by elexis on Sep 18 2019, 11:57 PM.

Details

Summary

Almost all of the files in source/gui/ match a classname that is implemented.
This file makes an important impression as it is named like the root folder GUI, yet it includes nothing really useful,
because many of the includes are not used by most files.
Additionally including "GUI.h" instead of gui/GUI.h is against the coding convention order of includes.
The file is also easily to confuse with CGUI.h which relates to an actually implemented important class.

Supersede typedef with using while at it.

File introduced in rP9, became this include-only file in rP28.

Test Plan

Make sure that it compiles, that all includes are needed, that no new useless includes are added.
Check for alphabetic include order and Coding Conventions includes.

Consider which classes are often enough used to be included in precompiled.h.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 18 2019, 11:57 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/750/display/redirect

elexis edited the test plan for this revision. (Show Details)Sep 18 2019, 11:58 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/238/display/redirect

nani awarded a token.Sep 19 2019, 12:08 AM
elexis added a subscriber: Angen.Sep 19 2019, 5:09 AM
elexis added inline comments.
source/pch/gui/precompiled.h
29 ↗(On Diff #9847)

@Angen got an opinion on which files should be here?

in source/gui/ there are basically only those two files used all over the place.

Most files in the directory are GUI object types, so some of them include IGUITextowner, IGUIScrollbarOwner, IGUIButtonbehavior. But if its only few, I wonder if its improving or worsening to add them here.

The patch looks good to me. Though I didn't test it.

source/gui/CButton.h
23 ↗(On Diff #9847)

Wrong order.

source/gui/CCheckBox.h
23 ↗(On Diff #9847)

Wrong order.

source/gui/CGUIScrollBarVertical.cpp
22 ↗(On Diff #9847)

I suggest to replace it by #include "gui/CGUI.h".

source/pch/gui/precompiled.h
29 ↗(On Diff #9847)

Wrong order?

I see two cases when to add header to precompiled.

  1. It is included in a lot of files ( what means more than 15 with small files)
  2. It is big file in terms of code and other includes and is used more than 5 times.
Stan added a subscriber: Stan.Sep 19 2019, 11:54 AM

Can any of this includes be replaced by a forward include ?

Angen added a comment.Sep 19 2019, 2:56 PM

builds in debug and without pch release

In D2304#96185, @Stan wrote:

Can any of this includes be replaced by a forward include ?

Nope:
The cpp files are the ones that actually use that type,
the header files can avoid it if its only stored as a pointer or function argument,
but not when it is an inherited class (like the I*.h files).
So one can quickly see that all of them are needed.

The ShaderProgramPtr.h would have looked like an omissible one, but the compiler complains about conflicting definition then.
Then if one looks at the file and sees that its already only the type defined there.

But there might be unrelated includes that could be removed in this folder, to be seen.

source/gui/CButton.h
23 ↗(On Diff #9847)

Ack, thanks. Reminds me I wanted to remove unused and sort all of them in this folder.

source/gui/CCheckBox.h
23 ↗(On Diff #9847)
source/gui/CGUIScrollBarVertical.cpp
22 ↗(On Diff #9847)

This happens in 3 places in this folder total, thanks. (Again, there are more issues with includes to be addressed)

source/gui/CList.h
22 ↗(On Diff #9847)

odeeer

source/gui/GUItypes.h
29 ↗(On Diff #9847)

This is new to include here, but seems cleaner than in every file that includes this file.

elexis edited the summary of this revision. (Show Details)Sep 20 2019, 3:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2019, 3:11 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 20 2019, 3:11 PM