Add STATIC_ASSERT_INCOMPLETE_TYPE to enforce include minimality.#107587
Add STATIC_ASSERT_INCOMPLETE_TYPE to enforce include minimality.#107587Repiteo merged 1 commit intogodotengine:masterfrom
STATIC_ASSERT_INCOMPLETE_TYPE to enforce include minimality.#107587Conversation
Ivorforce
force-pushed
the
static-assert-includes
branch
from
bef82a3 to
e5bf679
Compare
|
Is a good idea as it really needs discussion of its own. It looks fine to me (barring the question about the define above). |
|
We could expand this assert to headers by making the template reusable. The easiest way would probably be |
I'm not sure I understand, could you expand on the idea? Edit: Ohh, I think I get it. You mean probably something like this, right? (pseudocode) // <my_header.h>
bool was_already_incomplete = is_incomplete_type_v<Type>();
#include "abc.h"
#include "def.h"
#include "gi.h"
if (!was_already_incomplete && is_incomplete_type_v<Type>()) {
#error "Class was incomplete etc."
} |
Does that actually achieve anything that can't be done by putting this in the cpp? Apart from making the builds more likely to fail due to change in inclusion order? |
I don't think so. But it's clever! 😄 |
Ivorforce
force-pushed
the
static-assert-includes
branch
2 times, most recently
from
bfce35b to
9bb4311
Compare
lawnjelly
left a comment
lawnjelly
left a comment
There was a problem hiding this comment.
Looks fine to me, but this will need a check by @akien-mga .
There was a problem hiding this comment.
When suggesting __COUNTER__ for assertion reuse, this is what I was referring to:
// typedefs.h
template <typename T, int Counter, typename = void>
struct is_fully_defined : std::false_type {};
template <typename T, int Counter>
struct is_fully_defined<T, Counter, std::void_t<decltype(sizeof(T))>> : std::true_type {};
template <typename T, int Counter>
constexpr bool is_fully_defined_v = is_fully_defined<T, Counter>::value;
#if !defined(SCU_BUILD_ENABLED) && !defined(DEV_ENABLED) && !defined(DEBUG_ENABLED)
/// Enforces the requirement that a class is not fully defined.
/// This can be used to reduce include coupling and keep compile times low.
/// The check must be made at the top of the corresponding .cpp file of a header.
#define STATIC_ASSERT_INCOMPLETE_CLASS(m_type) \
class m_type; \
static_assert(!is_fully_defined_v<m_type, __COUNTER__>, #m_type " was unexpectedly fully defined. Please check the include hierarchy of '" __FILE__ "' and remove includes that resolve the class.");
#else
#define STATIC_ASSERT_INCOMPLETE_CLASS(m_type)
#endif
...
// ustring.cpp
#include "ustring.h"
STATIC_ASSERT_INCOMPLETE_CLASS(Dictionary); // Expectation: pass.
// Old: !is_fully_defined_v<Dictionary>. Passes.
// New: !is_fully_defined_v<Dictionary, 0>. Passes.
#include "core/variant/dictionary.h"
STATIC_ASSERT_INCOMPLETE_CLASS(Dictionary); // Expectation: fail.
// Old: !is_fully_defined_v<Dictionary>. Reuses previous template. Passes.
// New: !is_fully_defined_v<Dictionary, 1>. Distinct template. Fails.
Oh, that makes a lot more sense than what I thought you meant. |
Ivorforce
force-pushed
the
static-assert-includes
branch
from
9bb4311 to
12b7635
Compare
Ivorforce
changed the title
STATIC_ASSERT_INCOMPLETE_CLASS to enforce include minimality.STATIC_ASSERT_INCOMPLETE_TYPE to enforce include minimality.
There was a problem hiding this comment.
I appreciate the effort, reducing cross-includes is really important to bring down our compile times.
I'm a bit concerned about how many of these asserts we'll have to put everywhere to get good results, but I don't really have a better solution in mind.
Could use a rebase before merging, just to be somewhat up-to-date with master.
Add enforcements against `Dictionary` for `ustring.h` and two for `Dictionary` and `String` from `array.h`.
Ivorforce
force-pushed
the
static-assert-includes
branch
from
12b7635 to
712bc99
Compare
|
Thanks! |
4 participants
Cross-includes are our main compile time problem right now. Not only does parsing take long, also every small change to a header file leads to hundreds of recompilations of compile units. It's time to do something.
STATIC_ASSERT_INCOMPLETE_TYPEis designed to protect ourselves against unnecessary includes. It lets us enforce certain couplings are not made, such as an include todictionary.hfromustring.h. The assertion is made only for release build, to avoid interfering with the development cycle.(This check was originally made for #107492, but I split it off to make it easier to discuss it separately).
I enforce 3 assumptions in this PR that already hold (no
Dictionaryinclude fromString, andDictionaryandStringfromArray). There are many others we should be making. This is left to do as follow-up PRs (e.g. #106434).