{ ConfigKey::NecessaryMargin, ConfigModule::Navmesh },
{ ConfigKey::MaxCurvatureDerivative, ConfigModule::Autoreplanning },
⋮
/// still typing ///
Then you can write the logic once, iterating over the tuples of parameters.
for (auto [ck, cm] : the_list) {
if (key < ck) return cm;
}
// none of the above
return ConfigModule::Temtacle;
This is a general concept and working through it here will probably help you in more places.
Another idea, if you can be flexible about the assignment of code numbers, is to code the bunch in the high-order bits. So something like
enum ⋯ { NecessaryMargin= 0, ⋯
MaxCurvatureDerivative = 0x1'0000,
MaxLateralAcceleration, // continue auto-numbering
Now you can examine the high bits of the enumeration’s value to recover the module that goes with it.
if(moduleId < module_count)
{
return modules_[moduleId];
}
else
{
return std::nullopt;
}
Look at the code generated when you call this!
Writing it to allow NVRO is much cleaner.
std::optional<ConfigurationModule> retval;
if (moduleId < module_count)
retval.emplace(modules_[moduleId]);
return retval;
The default values could be built at compile time, not copied into a vector at run time. Try setting them up in sorted order in an array, and then find it using std::lower_bound etc. BTW, std::map uses a lot of (dynamic) memory and is slow; a sorted vector is faster for lookups!
Good luck with that project — it looks interesting!
{ ConfigKey::NecessaryMargin, ConfigModule::Navmesh },
{ ConfigKey::MaxCurvatureDerivative, ConfigModule::Autoreplanning },
/// still typing ///
{ ConfigKey::NecessaryMargin, ConfigModule::Navmesh },
{ ConfigKey::MaxCurvatureDerivative, ConfigModule::Autoreplanning },
⋮
Then you can write the logic once, iterating over the tuples of parameters.
for (auto [ck, cm] : the_list) {
if (key < ck) return cm;
}
// none of the above
return ConfigModule::Temtacle;
This is a general concept and working through it here will probably help you in more places.
Another idea, if you can be flexible about the assignment of code numbers, is to code the bunch in the high-order bits. So something like
enum ⋯ { NecessaryMargin= 0, ⋯
MaxCurvatureDerivative = 0x1'0000,
MaxLateralAcceleration, // continue auto-numbering
Now you can examine the high bits of the enumeration’s value to recover the module that goes with it.
if(moduleId < module_count)
{
return modules_[moduleId];
}
else
{
return std::nullopt;
}
Look at the code generated when you call this!
Writing it to allow NVRO is much cleaner.
std::optional<ConfigurationModule> retval;
if (moduleId < module_count)
retval.emplace(modules_[moduleId]);
return retval;
The default values could be built at compile time, not copied into a vector at run time. Try setting them up in sorted order in an array, and then find it using std::lower_bound etc. BTW, std::map uses a lot of (dynamic) memory and is slow; a sorted vector is faster for lookups!
Good luck with that project — it looks interesting!
using namespace std::literals::string_view_literals;
constexpr std::string_view config_values[]= {
"NavmeshObstaclesDilatation"sv, "LargestTriangleAreaInNavmesh"sv, ⋯
Note the sv on following the closing quote.
The advantage of making the array of string view, rather than plain lex strings, is that code does not have to call strlen when using them.
long ConfigurationHandler::getInt(ConfigKey key, ConfigModule module)
{
auto defaultValue = static_cast<int>(default_values_[static_cast<int>(key)].numeric_value);
auto sectionName = findSectionName(key, module);
return ini_reader_.GetInteger(sectionName, getKeyName(key), defaultValue);
}
double ConfigurationHandler::getDouble(ConfigKey key, ConfigModule module)
{
auto defaultValue = default_values_[static_cast<int>(key)].numeric_value;
auto sectionName = findSectionName(key, module);
return ini_reader_.GetReal(sectionName, getKeyName(key), defaultValue);
}
bool ConfigurationHandler::getBool(ConfigKey key, ConfigModule module)
{
auto defaultValue = default_values_[static_cast<int>(key)].boolean_value;
auto sectionName = findSectionName(key, module);
return ini_reader_.GetBoolean(sectionName, getKeyName(key), defaultValue);
⋯ etc. ⋯
I’m seeing a lot of repetition here, as well as conceptually a single operation that should be parameterized on type. You should make it a template, so most of the code is naturally shared. If ini_reader is a 3rd party library and not template friendly in this same way, you just have to supply one-line wrappers, not duplicate the whole body.
x = config.get<long>(key, module);
y = config.get<double> (key2, module);
z - config.get<bool> (key3, module);
Oh, and my syntax highlighting is reminding me that module is reserved as a new reserved word. So don’t use it as a variable name!
auto module = getModule(module_enum);
if(module)
{
module->registerCallback(callback);
}
The idiom is to initialize and test at the same time. The advantage is that the variable is only in scope if it is correct to use!
if (auto module = getModule(module_enum))
{
module->registerCallback(callback);
}
ConfigModule ConfigurationHandler::getModuleEnumFromKeyEnum(ConfigKey key) const noexcept
{
//I'm not proud of this function, but I could'nt find a better solution yet.
if(key < ConfigKey::NecessaryMargin)
{
return ConfigModule::Navmesh;
}
else if(key < ConfigKey::MaxCurvatureDerivative)
{
return ConfigModule::Autoreplanning;
}
else if(key < ConfigKey::NodeMemoryPoolSize)
{
return ConfigModule::ResearchMechanical;
}
else if(key < ConfigKey::PrecisionTrace)
{
return ConfigModule::Memory;
}
else
{
return ConfigModule::Tentacle;
}
}
Put the pairs of values in an array. So
{ ConfigKey::NecessaryMargin, ConfigModule::Navmesh },
{ ConfigKey::MaxCurvatureDerivative, ConfigModule::Autoreplanning },
/// still typing ///
constexpr std::string_view (
using namespace std::literals::string_view_literals;
constexpr std::string_view config_values[]= {
"NavmeshObstaclesDilatation"sv, "LargestTriangleAreaInNavmesh"sv, ⋯
Note the sv on following the closing quote.
The advantage of making the array of string view, rather than plain lex strings, is that code does not have to call strlen when using them.
long ConfigurationHandler::getInt(ConfigKey key, ConfigModule module)
{
auto defaultValue = static_cast<int>(default_values_[static_cast<int>(key)].numeric_value);
auto sectionName = findSectionName(key, module);
return ini_reader_.GetInteger(sectionName, getKeyName(key), defaultValue);
}
double ConfigurationHandler::getDouble(ConfigKey key, ConfigModule module)
{
auto defaultValue = default_values_[static_cast<int>(key)].numeric_value;
auto sectionName = findSectionName(key, module);
return ini_reader_.GetReal(sectionName, getKeyName(key), defaultValue);
}
bool ConfigurationHandler::getBool(ConfigKey key, ConfigModule module)
{
auto defaultValue = default_values_[static_cast<int>(key)].boolean_value;
auto sectionName = findSectionName(key, module);
return ini_reader_.GetBoolean(sectionName, getKeyName(key), defaultValue);
⋯ etc. ⋯
I’m seeing a lot of repetition here, as well as conceptually a single operation that should be parameterized on type. You should make it a template, so most of the code is naturally shared. If ini_reader is a 3rd party library and not template friendly in this same way, you just have to supply one-line wrappers, not duplicate the whole body.
x = config.get<long>(key, module);
y = config.get<double> (key2, module);
z - config.get<bool> (key3, module);
Oh, and my syntax highlighting is reminding me that module is reserved as a new reserved word. So don’t use it as a variable name!
auto module = getModule(module_enum);
if(module)
{
module->registerCallback(callback);
}
The idiom is to initialize and test at the same time. The advantage is that the variable is only in scope if it is correct to use!
if (auto module = getModule(module_enum))
{
module->registerCallback(callback);
}
ConfigModule ConfigurationHandler::getModuleEnumFromKeyEnum(ConfigKey key) const noexcept
{
//I'm not proud of this function, but I could'nt find a better solution yet.
if(key < ConfigKey::NecessaryMargin)
{
return ConfigModule::Navmesh;
}
else if(key < ConfigKey::MaxCurvatureDerivative)
{
return ConfigModule::Autoreplanning;
}
else if(key < ConfigKey::NodeMemoryPoolSize)
{
return ConfigModule::ResearchMechanical;
}
else if(key < ConfigKey::PrecisionTrace)
{
return ConfigModule::Memory;
}
else
{
return ConfigModule::Tentacle;
}
}
Put the pairs of values in an array. So
{ ConfigKey::NecessaryMargin, ConfigModule::Navmesh },
{ ConfigKey::MaxCurvatureDerivative, ConfigModule::Autoreplanning },
/// still typing ///
//The array that keeps the string values of the ConfigKeys
const std::string configuration_key_string_values[configuration_key_count] = {
"NavmeshObstaclesDilatation", "LargestTriangleAreaInNavmesh", "LongestEdgeInNavmesh", "NavmeshFilename", ⋯
When I have an array like this for names or other purpose, where the actual enumeration values are significant, I will use at least an explicit initializer on the first one (as did you) but also comments there explaining that. Furthermore, all places that must be updated as well when you change the enumeration will be marked with some unique word that can be grepped for (and that is part of the commentary).
You might be interested in the short video "Enums 4 Ways" at CppCon (née BoostCon) 2018, posted to YouTube last week.
Do the enumeration values need to be a contiguous range of small integers (e.g. used in switch statements, stored in small words) or just serve as compile-time unique values?
Because another thing you can do is define named constants rather than enumerators. If the C++ symbol is itself a lex string, you automatically have the correspondence at least in one direction!
struct my_enum_thing {
static const char* const NavmeshObstaclesDilatation = "NavmeshObstaclesDilatation";
// etc.
The actual value of the symbol is a pointer, and unique. But, they are the full word size, and not consecutive numbers.
You might use an "X Macro" approach to define everything from one supplied list of names.
const std::string configuration_key_string_values[configuration_key_count] =
You don’t need to give the array size (and have those previous lines to figure it out), as it will simply match the initializer count.
You are creating all the string objects at run-time, which will duplicate the bytes of the string. So you will have the original lex strings still in your read-only data, and the std::strings with pointers to heap memory containing the same contents. And all that copying has to happen at program-start.
Make the array a constexpr itself, and use std::string_view values. You can construct the string view wrapper at compile time, and it points back to the lex string, without duplication.
constexpr std::string_view (