-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove GenericConfigurationManagerImpl's CRTP #11784
Remove GenericConfigurationManagerImpl's CRTP #11784
Conversation
…manager's as a protected interface. Add the config class as a template parameter.
This fixes the doxygen generation.
PR #11784: Size comparison from 2832392 to 17560fc Increases above 0.2%:
Increases (32 builds for efr32, esp32, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (6 builds for linux)
Full report (35 builds for efr32, esp32, linux, mbed, nrfconnect, p6, qpg, telink)
|
PR #11784: Size comparison from eb40a4c to 186d097 Increases above 0.2%:
Increases (35 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (6 builds for linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
Glad to see we are moving this direction. Using CRTP may save us several hundred bytes memory, but its complexity and error prone is not fitting for the reference SDK of CHIP. |
fast track: this is generally a refactor for vtable/crtp change. It should not affect functionality. PR has been up for sufficient time for cross-timezone review. |
* Explicitly add the config value methods to the generic configuration manager's as a protected interface. Add the config class as a template parameter. * Remove the ImplClass from the GenericConfigurationManagerImpl template. * Minor cleanup. * Restyling with clang-format. * Apply the same changes to the other platforms. * Fix Linux with the same changes. * Fix remaining platforms. * Remove the Key namespacing since it's already defined in the superclass. This fixes the doxygen generation. * Remove the explicit template definitions.
Problem
The CRTP in the GenericConfigurationManagerImpl hides an interface that it depends on, as well as makes it more challenging to make subclasses of the platform-specific ConfigurationManager implementations.
Change overview
Testing
Ran the unit tests.