-
Notifications
You must be signed in to change notification settings - Fork 88
Remove deprecated score::StringLiteral #745
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,9 +99,7 @@ abstract class "mw::com::impl::IRuntime" { | |
|
|
||
| class "mw::com::impl::Runtime" { | ||
| -binding_runtimes_ : std:unordered_map<BindingType, std::unique_ptr<IBindingRuntime>> | ||
| {static} +Initialize() : void | ||
| {static} +Initialize(int argc, score::StringLiteral argv) : void | ||
| {static} +Initialize(std::string const&) : void | ||
| {static} +Initialize(runtime::RuntimeConfiguration&) : void | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this changes to a separate PR together with https://github.com/eclipse-score/communication/pull/745/changes#r3656356347? That is not really a change in the API, it is just fixing the puml that was wrong. |
||
| {static} +getInstance() : Runtime& | ||
| +Runtime(Configuration&& config) | ||
| +resolve(const InstanceSpecifier&) : std::vector<InstanceIdentifier> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,11 +14,9 @@ | |||||||
| #define SCORE_MW_COM_MOCKING_I_RUNTIME_H | ||||||||
|
|
||||||||
| #include "score/mw/com/runtime_configuration.h" | ||||||||
|
|
||||||||
| #include "score/mw/com/types.h" | ||||||||
|
|
||||||||
| #include "score/result/result.h" | ||||||||
| #include "score/string_manipulation/string_literal.h" | ||||||||
|
|
||||||||
| #include <score/span.hpp> | ||||||||
|
|
||||||||
|
|
@@ -34,7 +32,11 @@ class IRuntime | |||||||
| virtual ~IRuntime() = default; | ||||||||
|
|
||||||||
| virtual score::Result<InstanceIdentifierContainer> ResolveInstanceIDs(const InstanceSpecifier model_name) = 0; | ||||||||
| virtual void InitializeRuntime(const std::int32_t argc, score::cpp::span<const score::StringLiteral> argv) = 0; | ||||||||
| [[deprecated( | ||||||||
| "Please use InitializeRuntime(cpp::span<safecpp::zstring_view> command_line_arguments) for guaranteed NULL " | ||||||||
| "terminated arguments")]] | ||||||||
| virtual void InitializeRuntime(const std::int32_t argc, score::cpp::span<const char*> argv) = 0; | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
If we add the deprecation attribute also in the interface it also serve us as a reminder that this is deprecated. |
||||||||
| virtual void InitializeRuntime(const cpp::span<safecpp::zstring_view> command_line_arguments) = 0; | ||||||||
| virtual void InitializeRuntime(const runtime::RuntimeConfiguration& runtime_configuration) = 0; | ||||||||
|
|
||||||||
| protected: | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,6 @@ | |
| #include "score/mw/com/runtime_configuration.h" | ||
|
|
||
| #include "score/filesystem/path.h" | ||
| #include "score/string_manipulation/string_literal.h" | ||
|
|
||
| #include <gmock/gmock.h> | ||
| #include <gtest/gtest.h> | ||
|
|
@@ -82,17 +81,18 @@ TEST_F(RuntimeMockFixture, ResolveInstanceIdsReturnsErrorWhenMockReturnsError) | |
| EXPECT_EQ(resolve_instance_ids_result.error(), error_code); | ||
| } | ||
|
|
||
| // TODO: Once InitializeRuntime(argc, argv) is removed, this test will also be removed | ||
| TEST_F(RuntimeMockFixture, InitializeRuntimeDispatchesToMockAfterInjectingMock) | ||
| { | ||
| // Given that a mocked runtime has been injected | ||
|
|
||
| // Expecting that InitializeRuntime will be called on the mock with the same argc / argv that is passed to | ||
| // InitializeRuntime (where argv has been converted to an score::cpp::span | ||
| constexpr std::int32_t argc{1U}; | ||
| score::StringLiteral argv[] = {"some_argument"}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a note to this test mentioning that once the deprecated InitializeRuntime(argc, argv) is removed, the test needs to be removed also. This helps to identify that these char* here are only needed for testing this deprecated API. |
||
| const char* argv[] = {"some_argument"}; | ||
| EXPECT_CALL(runtime_mock_, InitializeRuntime(argc, _)).WillOnce(Invoke([argv](auto, auto argv_span) { | ||
| const score::cpp::span<const score::StringLiteral> expected_argv_span( | ||
| argv, static_cast<score::cpp::span<const score::StringLiteral>::size_type>(argc)); | ||
| const score::cpp::span<const char*> expected_argv_span( | ||
| const_cast<const char**>(argv), static_cast<score::cpp::span<const char*>::size_type>(argc)); | ||
| ASSERT_EQ(argv_span.size(), expected_argv_span.size()); | ||
| for (std::size_t i = 0; i < argv_span.size(); ++i) | ||
| { | ||
|
|
@@ -104,15 +104,40 @@ TEST_F(RuntimeMockFixture, InitializeRuntimeDispatchesToMockAfterInjectingMock) | |
| InitializeRuntime(argc, argv); | ||
| } | ||
|
|
||
| TEST_F(RuntimeMockFixture, InitializeRuntimeWithZStringViewsDispatchesToMockAfterInjectingMock) | ||
| { | ||
| // Given that a mocked runtime has been injected | ||
|
|
||
| // Expecting that InitializeRuntime will be called on the mock with the same span that is passed to | ||
| // InitializeRuntime | ||
| using safecpp::literals::operator""_zsv; | ||
| constexpr auto kDummyApplicationNameZsv = "dummyname"_zsv; | ||
| safecpp::zstring_view argv[] = {kDummyApplicationNameZsv}; | ||
| score::cpp::span<safecpp::zstring_view> argv_span(argv, std::size(argv)); | ||
|
|
||
| EXPECT_CALL(runtime_mock_, InitializeRuntime(An<cpp::span<safecpp::zstring_view>>())) | ||
| .WillOnce(Invoke([argv_span](auto arguments_span) { | ||
| ASSERT_EQ(argv_span.size(), arguments_span.size()); | ||
| for (std::size_t i = 0; i < argv_span.size(); ++i) | ||
| { | ||
| EXPECT_EQ(argv_span.data()[i], arguments_span.data()[i]); | ||
| } | ||
| })); | ||
|
|
||
| // When calling InitializeRuntime | ||
| InitializeRuntime(argv); | ||
| } | ||
|
|
||
| TEST_F(RuntimeMockFixture, InitializeRuntimeWithRuntimeConfigurationDispatchesToMockAfterInjectingMock) | ||
| { | ||
| // Given that a mocked runtime has been injected | ||
|
|
||
| // Expecting that InitializeRuntime will be called on the mock | ||
| RuntimeConfiguration runtime_configuration{kDummyConfigurationPath}; | ||
| EXPECT_CALL(runtime_mock_, InitializeRuntime(_)).WillOnce(Invoke([](auto& runtime_configuration) { | ||
| EXPECT_EQ(runtime_configuration.GetConfigurationPath(), kDummyConfigurationPath); | ||
| })); | ||
| EXPECT_CALL(runtime_mock_, InitializeRuntime(An<const RuntimeConfiguration&>())) | ||
| .WillOnce(Invoke([](auto& runtime_configuration) { | ||
| EXPECT_EQ(runtime_configuration.GetConfigurationPath(), kDummyConfigurationPath); | ||
| })); | ||
|
|
||
| // When calling InitializeRuntime | ||
| InitializeRuntime(runtime_configuration); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,9 +14,7 @@ | |||||||
|
|
||||||||
| #include "score/filesystem/path.h" | ||||||||
| #include "score/mw/log/logging.h" | ||||||||
| #include "score/string_manipulation/string_literal.h" | ||||||||
|
|
||||||||
| #include <score/assert.hpp> | ||||||||
| #include <score/span.hpp> | ||||||||
|
|
||||||||
| #include <cstddef> | ||||||||
|
|
@@ -45,10 +43,26 @@ RuntimeConfiguration::RuntimeConfiguration(filesystem::Path configuration_path) | |||||||
| } | ||||||||
|
|
||||||||
| // NOLINTNEXTLINE(modernize-avoid-c-arrays):C-style array tolerated for command line arguments | ||||||||
| RuntimeConfiguration::RuntimeConfiguration(const std::int32_t argc, score::StringLiteral argv[]) : configuration_path_{} | ||||||||
| RuntimeConfiguration::RuntimeConfiguration(const std::int32_t argc, const char* argv[]) : configuration_path_{} | ||||||||
| { | ||||||||
| // We convert the const char* arguments into safecpp::zstring_views to ensure that they are null-terminated and safe | ||||||||
| // to use. | ||||||||
| // This is a deprecated API for a reason! If the given argv[] is not null-terminated, it can lead to undefined | ||||||||
| // behavior. But this isn't introduced newly by this conversion! | ||||||||
| std::vector<safecpp::zstring_view> command_line_arguments{}; | ||||||||
| for (std::int32_t arg_idx = 0U; arg_idx < argc; arg_idx++) | ||||||||
| { | ||||||||
| auto argument = std::string_view{argv[arg_idx]}; | ||||||||
| command_line_arguments.push_back(safecpp::zstring_view{argument.data(), argument.size()}); | ||||||||
| } | ||||||||
|
|
||||||||
| auto configuration_path = ParseConfigurationPath(command_line_arguments); | ||||||||
| configuration_path_ = | ||||||||
| configuration_path.has_value() ? std::move(configuration_path).value() : kDefaultConfigurationPath; | ||||||||
| } | ||||||||
|
|
||||||||
| RuntimeConfiguration::RuntimeConfiguration(cpp::span<safecpp::zstring_view> command_line_arguments) | ||||||||
| { | ||||||||
| const score::cpp::span<const score::StringLiteral> command_line_arguments( | ||||||||
| argv, static_cast<score::cpp::span<const score::StringLiteral>::size_type>(argc)); | ||||||||
| auto configuration_path = ParseConfigurationPath(command_line_arguments); | ||||||||
| configuration_path_ = | ||||||||
| configuration_path.has_value() ? std::move(configuration_path).value() : kDefaultConfigurationPath; | ||||||||
|
|
@@ -60,15 +74,16 @@ const filesystem::Path& RuntimeConfiguration::GetConfigurationPath() const& | |||||||
| } | ||||||||
|
|
||||||||
| std::optional<filesystem::Path> RuntimeConfiguration::ParseConfigurationPath( | ||||||||
| const score::cpp::span<const score::StringLiteral> command_line_args) | ||||||||
| const score::cpp::span<safecpp::zstring_view> command_line_args) | ||||||||
| { | ||||||||
| const auto num_args = command_line_args.size(); | ||||||||
|
|
||||||||
| std::optional<std::string> configuration_path{}; | ||||||||
| for (std::uint32_t arg_idx = 0U; arg_idx < num_args; arg_idx++) | ||||||||
| { | ||||||||
| // TODO: Adapt code that we do not need to call `.data()` and that we do not have to rely on life time extension | ||||||||
| const std::string& command_line_argument_key{ | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| score::cpp::at(command_line_args, static_cast<std::ptrdiff_t>(arg_idx))}; | ||||||||
| score::cpp::at(command_line_args, static_cast<std::ptrdiff_t>(arg_idx)).data()}; | ||||||||
| if (command_line_argument_key == kConfigurationPathCommandLineKey) | ||||||||
| { | ||||||||
| const auto index_of_configuration_path = arg_idx + 1U; | ||||||||
|
|
@@ -79,7 +94,8 @@ std::optional<filesystem::Path> RuntimeConfiguration::ParseConfigurationPath( | |||||||
| << "\" but no corresponding value. Terminating."; | ||||||||
| std::terminate(); | ||||||||
| } | ||||||||
| return score::cpp::at(command_line_args, static_cast<std::ptrdiff_t>(index_of_configuration_path)); | ||||||||
| // TODO: Adapt code that we do not need to call `.data()` and we can provide zstring_view to a filepath | ||||||||
| return score::cpp::at(command_line_args, static_cast<std::ptrdiff_t>(index_of_configuration_path)).data(); | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| } | ||||||||
| if (command_line_argument_key == kDeprecatedConfigurationPathCommandLineKey) | ||||||||
| { | ||||||||
|
|
@@ -93,7 +109,8 @@ std::optional<filesystem::Path> RuntimeConfiguration::ParseConfigurationPath( | |||||||
| << "\" but no corresponding value. Terminating."; | ||||||||
| std::terminate(); | ||||||||
| } | ||||||||
| return score::cpp::at(command_line_args, static_cast<std::ptrdiff_t>(index_of_configuration_path)); | ||||||||
| // TODO: Adapt code that we do not need to call `.data()` and we can provide zstring_view to a filepath | ||||||||
| return score::cpp::at(command_line_args, static_cast<std::ptrdiff_t>(index_of_configuration_path)).data(); | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| } | ||||||||
| } | ||||||||
| return configuration_path; | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is interesting, I do not get why you need to change it and you do not have to add it. I would expect that if we have both (the new overload and the deprecated API) then you have to add one and keep also the old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@castler this looks like the API surface checker has issues with overloads.