SAnsell / CombLayer

MCNP(X) project builder using C++
GNU General Public License v3.0
14 stars 13 forks source link

Importance class prototype is not defined in System/monteInc/Object.h #122

Closed rodionk1 closed 3 years ago

rodionk1 commented 3 years ago

Plugging in some new components with master branch as a starting point I have experienced that the code doesn't compile if #include"Object.h" is not preceded with #include"Importance.h".

SAnsell commented 3 years ago

That is 100% intended. [or it is UNLESS it is in the current branch that doesn't compile] If you are making your own classes this is 100% intended to happen. So I will explain why.

There are two types of C++ include design methods. Called explicit include and nested include. Consider class A that contains class B, both defined in separate files. Effectively, you have to have #include "B.h" followed by #include "A.h".

Nested include : Many project, (including the stl) add #include "B.h" within file: "A.h". This has the advantage that you don't have to think when you add each include to you main project BUT it has two significant drawbacks. First, REGARDLESS of putting #ifndef A_H #define A_H and #endif (*** pragma_once see below) around the include [which means it is parse only once. The compile STILL has to load each file. That is because the #includes are incorporated by the pre-processor to make a single text stream. The pre-processor has to read each file each time. It is no problem if you have only two files [doesn't change anything]. But if you have multi dependency (e.g. C also include B) then it increases size of the read and buffers, very fast.

Explicit include This occurs when you dis-allow (or choose not to) put #include in other include files. Obviously, you need a long list of #include in most object files.

Scientific code (which CombLayer nearly is) has high dependency on base classes (e.g. Vec3D/ Matrix), because it is natural to write classes operating on these kind of objects. The dependency track of that is sufficient to slow the compilation down to such an extent that compile time become significant. Try to compile mantid (Neutron/Xray visualization code). The change one file and see how long it takes. That factor 10 adds up constantly.

The second point is I want people to KNOW what is needed. Consider that you want to find a bug / or add a feature. I expect that you don't know the code (or the part of the code). Most people just work in Model, and when they need something from System. It is 100% going to be from the list of includes in the file. You avoid the include dependency hell, that much of the cern/STL code is [well written code, but finding anything is opening file, then a file and then an file, just to find a three line implementation. ]

So in short, it is a trade off, if you need the users to either use the program more like a black box, or (in the case of Mantid), it is normally a compile once and use (much more mature project). Then the nested include is fine. But I have not yet been convinced that CombLayer should go that route.

Further regardless of going to a nested include OR and explicit include form, I think that all projects should choose one and be consistent throughout. So for the reasons I currently believe are good [If you want to discuss, tell me why I am wrong PLEASE DO!! Most of the decisions in CombLayer were taken exclusively -- which means most will be sub-optimal.] until we change I want to keep with one approach and currently that is explicit includes.

*** Pragma_once is an improvement over the #define guard. It preventes some of the read-all problem but not all. Technically it is not standard, but in practice it is in clang, gcc, visual studio, ibm (portland), etc so it is.

rodionk1 commented 3 years ago

Thanks for the explicit explanation!

SAnsell commented 3 years ago

Thanks, I copied the answer over to discussion because once an issue is closed it kind of gets buried. I am sure someone else either wondered, or will ask in future.

However, thanks for asking, I am happy to explain my design decisions [and often will see that other issues have resulted in wholesale changes, where we got it wrong].

On Fri, 29 Jan 2021 at 11:53, Rodion Kolevatov notifications@github.com wrote:

Closed #122 https://github.com/SAnsell/CombLayer/issues/122.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/SAnsell/CombLayer/issues/122#event-4265793743, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADE4Q2NQDRCHYQXG7IZN33S4KOSZANCNFSM4WYUPMUA .