Details
- Reviewers
Stan Itms - Commits
- rP21788: Clean some forward declarations.
Ensure that a forward declaration is used in and only in header files that don't require many related JS declarations or members such as CxPrivate.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5969 Build 9958: Vulcan Build Jenkins Build 9957: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/436/display/redirect
Your feeling was right:
In header files, avoid #include and use forward declarations wherever possible.
Should I guess to which line you refering (JSInterface_Console.h) to and answer that (just because something else includes the scriptinterface member declarations doesn't mean that we should leave it out there) or is anyone willing to precise that statement before I do that?
and in the other files you should delete the include rather than the forward declaration if possible.
This line can be removed entirely, but we shall not rely on ScriptFunctions.cpp providing the declaration, right?
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | @Itms we, 1. use a forward declaration for a class if we don't reference private members or other declaration of the header file,
|
I am not sure I understood, so to clear misunderstandings I wrote what Stan, Imarok and me are saying should be the correct fix.
What do you mean by "rely" on a cpp file?
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | I am not sure I understand what you mean by all those points. A forward declaration is enough if the header only contains pointers and references to instances of the class. If we have actual instances, an include is needed. The CC suggest to use forwards declarations, so that changes to headers do not trigger useless recompilations of code. | |
21 | No, don't change this one. | |
source/ps/scripting/JSInterface_Mod.h | ||
22 | No, delete the include instead. | |
source/ps/scripting/JSInterface_ModIo.h | ||
21 | No, delete both includes instead. |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 |
That seems incorret / incomplete: |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | Hm then why does JSInterface_Console.h currently works? As far as I can see it only has a forward declaration and pointers to ScriptInterface::CxPrivate. It rather sounds like the cpp file that includes JSInterface_ModIo.h is missing the scriptinterface/ScriptInterface.h include. |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | JSInterface_Console.h is included in three places and at least one (GameSetup.cpp) includes it prior the ScriptInterface to keep alphabetic ordering according to the coding conventions, which is an example on why it is good practice to serve declarations in header files, right? |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | I'm sorry I don't see your point... You know that forward declarations are here so that we can put includes in any order, right? And I don't know what you mean by "serv[ing] declarations in header files". In a nutshell, the more includes in .cpp files and the less in .h files, the better. Is it clearer for you put that way? Do you see the rationale? |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | I agree with what you state here, but not with the conclusion to keep this line as is. The line should be either removed entirely because ScriptInterface and ScriptInterface::CxPrivate are both declared prior (it does compile without this line), or we change this to an actual include, so that both ScriptInterface and ScriptInterface::CxPrivate are declared independent of prior declarations. I would agree to use a forward include here, if we would only have a reference to a ScriptInterface type. But this file also has a declaration of ScriptInterface::CxPrivate which is not declared by the forward include. We can see in the other file that a forward include class ScriptInterface; is insufficient to mark ScriptInterface::CxPrivate as declared (Reproduce by removing the include but keeping the forward declaration in JSInterface_Modio.h and get ../../../source/ps/scripting/JSInterface_ModIo.h:30:50: error: ‘pCxPrivate’ was not declared in this scope). The forward include here is, as is, entirely useless, the line can be removed and it still compiles. It only compiles because something else already happens to include Scriptinterface.h which declares both ScriptInterface and ScriptInterface::CxPrivate. |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | Or let me put it this way, |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | A pointer to an element of the ScriptInterface namespace is covered by a forward declaration of the class. The compiler error is because JSInterface_ModIo.cpp (CPP) should have #include "scriptinterface/ScriptInterface.h". I'm sorry I can't compile that, I'm at work. I don't think I need to do it myself though. The forward declaration might be useless by chance in this specific situation, but it is needed if you want to be free to include headers in any order. Also, please use correct denomination for things, in order to overcome the misunderstandings: a "forward include" doesn't exist, I don't know if you are referring to a "forward declaration" or a "header include". |
source/ps/scripting/JSInterface_Console.h | ||
---|---|---|
21 | Answering the filesearch comment: yes, indeed, we are doing it wrong currently. It builds but it is more difficult to maintain. Your current patch goes from 90% going against the conventions to 100% against. I don't think it's the end of the world if we stay at 90%, but you're not going in the good direction. Removing the forward declaration would be an actual C++ mistake though, as it would cause future changes to break. |
After clarification on IRC, I was wrong about thinking that a forward declaration would be enough for ScriptInterface::CxPrivate*. The current code only works by chance, because JSInterface_Console.cpp has a specific include ordering.