Skip to content
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

[config] Implementation of new configuration management #1505

Merged
merged 120 commits into from
Jun 11, 2024

Conversation

Peguen
Copy link
Contributor

@Peguen Peguen commented Apr 4, 2024

Description

Introduced config management via structs which opens the possibilty for users to configure eCAL in code. In addition, evaluation of the inputs will be easily possible.

If assigned configuration values can be evaluated and are incorrect, the program will exit with an error.

Internally also the new config structs will be used.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

ecal/core/src/ecal_global_accessors.cpp Show resolved Hide resolved
ecal/core/src/ecal_global_accessors.h Show resolved Hide resolved
Configuration::Configuration() :
share_topic_type(eCAL::Config::IsTopicTypeSharingEnabled()),
share_topic_description(eCAL::Config::IsTopicDescriptionSharingEnabled())
Configuration::Configuration()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: constructor does not initialize these fields: tcp, share_topic_type, share_topic_description [cppcoreguidelines-pro-type-member-init]

ecal/core/include/ecal/config/ecal_publisher_config.h:137:

-       TCP::Configuration   tcp;
+       TCP::Configuration   tcp{};

ecal/core/include/ecal/config/ecal_publisher_config.h:139:

-       bool                 share_topic_type;                            //!< share topic type via registration
-       bool                 share_topic_description;                     //!< share topic description via registration
+       bool                 share_topic_type{};                            //!< share topic type via registration
+       bool                 share_topic_description{};                     //!< share topic description via registration

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Configuration::Configuration() :
share_topic_type(eCAL::Config::IsTopicTypeSharingEnabled()),
share_topic_description(eCAL::Config::IsTopicDescriptionSharingEnabled())
Configuration::Configuration()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: constructor does not initialize these fields: tcp, share_topic_type, share_topic_description [cppcoreguidelines-pro-type-member-init]

ecal/core/include/ecal/config/publisher.h:137:

-       TCP::Configuration   tcp;
+       TCP::Configuration   tcp{};

ecal/core/include/ecal/config/publisher.h:139:

-       bool                 share_topic_type;                            //!< share topic type via registration
-       bool                 share_topic_description;                     //!< share topic description via registration
+       bool                 share_topic_type{};                            //!< share topic type via registration
+       bool                 share_topic_description{};                     //!< share topic description via registration

@Peguen Peguen changed the title [DRAFT][config] Implementation of new configuration management [config] Implementation of new configuration management May 28, 2024
@Peguen Peguen marked this pull request as ready for review May 28, 2024 11:37
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

std::string ecal_ini_file_path{};

private:
ECAL_API void Init(std::vector<std::string>& args_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'eCAL::Configuration::Init' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

          ECAL_API void Init(std::vector<std::string>& args_);
                        ^
Additional context

ecal/core/src/config/ecal_config_initializer.cpp:238: the definition seen here

    void Configuration::Init(std::vector<std::string>& arguments_)
                        ^

ecal/core/include/ecal/config/configuration.h:77: differing parameters are named here: ('args_'), in definition: ('arguments_')

          ECAL_API void Init(std::vector<std::string>& args_);
                        ^


eCAL::Config::CmdParser parser;

std::vector<std::string> arguments;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'arguments' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::string> arguments;
std::vector<std::string> arguments = 0;

{
eCAL::Config::CmdParser parser;

std::vector<std::string> arguments;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'arguments' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<std::string> arguments;
std::vector<std::string> arguments = 0;

Copy link
Contributor

@hannemn hannemn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed together with @Peguen. Further restructuring of configuration object is needed but can be handled in a subsequent PR.

@hannemn hannemn merged commit 5122562 into eclipse-ecal:master Jun 11, 2024
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants