You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Implement Settings Aliases without nested calls (fixes clang 20 build)
The problem in the current implementation is that it does the following:
struct DefineAliases
{
DefineAliases & setName(std::string_view value);
DefineAliases & addAlias(std::string_view value);
};
DefineAlises()
.setName()
.addAlias()
And even though it looks very **natural and clean**, there is a problem
with such approach.
Right now ClickHouse has 1252 settings, so it will have 1252 nested
setName() calls and bunch of addAlias() calls (for those settings which
has aliases)
And this bails out on clang 20 on arm64 (likely only there because the
each frame is heavier due to larger number of registers):
/ch/src/Core/Settings.cpp:6713:1: error: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely [-Werror,-Wstack-exhausted]
6713 | DECLARE_SETTINGS_TRAITS_ALLOW_CUSTOM_SETTINGS(SettingsTraits, LIST_OF_SETTINGS)
| ^
/ch/src/Core/BaseSettings.h:885:5: note: expanded from macro 'DECLARE_SETTINGS_TRAITS_ALLOW_CUSTOM_SETTINGS'
885 | DECLARE_SETTINGS_TRAITS_COMMON(SETTINGS_TRAITS_NAME, LIST_OF_SETTINGS_MACRO, 1)
| ^
/ch/src/Core/BaseSettings.h:946:52: note: expanded from macro 'DECLARE_SETTINGS_TRAITS_COMMON'
946 | DefineAliases() LIST_OF_SETTINGS_MACRO(ALIAS_TO, ALIAS_FROM); \
So instead of this we can simply create the std::unordered_map for
aliases using initializer list, but, for this I have to change the
declaration, since with the chain of DECLARE() ALIAS() it is not
possible to do, so now there is DECLARE() and DECLARE_WITH_ALIAS(), the
later should be called instead if the setting has alias.
This will also to skip settings without aliases during
aliases_to_settings initialization, which should be a good thing.
P.S. Thanks to Rual new settings encapsulated so good that this changes
did not require recompile everything!
0 commit comments