Use shared error type map #214
Conversation
| @@ -14,13 +15,12 @@ namespace Everest { | |||
| namespace error { | |||
|
|
|||
| struct ErrorDatabase; | |||
| struct ErrorTypeMap; | |||
There was a problem hiding this comment.
I don't really get what the idea behind all the forward defines was - I've removed it to just use the ErrorTypeMapPtr type
There was a problem hiding this comment.
The idea of forward declarations is to avoid including dependencies in the header file to reduce compile times after changing something in the included files
Signed-off-by: Dima Dorezyuk <ddo@qwello.eu>
dorezyuk
force-pushed
the
feat/use_shared_error_map
branch
from
382a3ca to
1dd7579
Compare
Signed-off-by: Dima Dorezyuk <ddo@qwello.eu>
dorezyuk
force-pushed
the
feat/use_shared_error_map
branch
from
4f4d37a to
a8d57b5
Compare
There was a problem hiding this comment.
Thanks for your work here @dorezyuk,
I really would like to merge the optimization of sharing the pointer instead of copying the ErrorTypeMap.
Could you please move the style changes in another PR/Discussion?
Regarding the forward declarations I see a disadvantage in removing them since they can safe compile time by reducing dependencies between the header files.
Please let me know if you have further questions to my comments
| /// Const pointer to a ErrorTypeMap. This is the default how we share the | ||
| /// error type map between different classes. | ||
| using ConstPtr = std::shared_ptr<const ErrorTypeMap>; | ||
|
|
There was a problem hiding this comment.
I would prefer to keep using std::shared_ptr<const ErrorTypeMap> since it fits the style arround here
There was a problem hiding this comment.
not sure if i understand - we're using it
| @@ -5,6 +5,9 @@ | |||
| #define UTILS_ERROR_TYPE_MAP_HPP | |||
|
|
|||
| #include <filesystem> | |||
| #include <map> | |||
| #include <memory> | |||
There was a problem hiding this comment.
| #include <memory> |
This dependency can be avoided, when not defining this ConstPtr
There was a problem hiding this comment.
also not sure if I understand - we use it in this library a lot. What do you mean with "avoid" the dependency? Are you concerned about the preprocessor adding this?
|
|
||
| namespace Everest { | ||
| namespace error { | ||
|
|
||
| struct ErrorTypeMap; |
There was a problem hiding this comment.
I would like to keep this forward declaration to avoid unnecessary dependencies in all these header files
| @@ -14,13 +15,12 @@ namespace Everest { | |||
| namespace error { | |||
|
|
|||
| struct ErrorDatabase; | |||
| struct ErrorTypeMap; | |||
There was a problem hiding this comment.
The idea of forward declarations is to avoid including dependencies in the header file to reduce compile times after changing something in the included files
| @@ -397,7 +397,7 @@ std::tuple<json, int> Config::load_and_validate_with_schema(const fs::path& file | |||
| Config::Config(std::shared_ptr<RuntimeSettings> rs) : Config(rs, false) { | |||
| } | |||
|
|
|||
| Config::Config(std::shared_ptr<RuntimeSettings> rs, bool manager) : rs(rs), manager(manager) { | |||
| Config::Config(std::shared_ptr<RuntimeSettings> rs, bool manager) : rs(rs), manager(manager), error_map{nullptr} { | |||
There was a problem hiding this comment.
This default value is not needed, especially because it is initialized in the body of the constructor
| this->impl_error_managers[impl] = | ||
| std::make_shared<error::ErrorManagerImpl>(this->config.get_error_map(), error_database, allowed_error_types, | ||
| publish_raised_error, publish_cleared_error); |
There was a problem hiding this comment.
I don't really get what changed here
Currently the error type map is copy constructed and then placed into a shared pointer which results in the error map not being really shared between the different classes. This Pr fixes it.