From 728d3b4b980d29637d9b528548284e5772b45dd5 Mon Sep 17 00:00:00 2001 From: jonawals Date: Fri, 28 May 2021 12:33:27 +0100 Subject: [PATCH 1/2] Address PR comments --- .../Source/TestImpactCommandLineOptions.cpp | 10 +++++----- .../Source/TestImpactCommandLineOptions.h | 4 ++-- .../TestImpactCommandLineOptionsUtils.cpp | 20 ++++++++++--------- .../TestImpactCommandLineOptionsUtils.h | 10 +++++----- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp index e4083641e5..a6eb2e886e 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp @@ -184,8 +184,8 @@ namespace TestImpact Policy::TargetOutputCapture ParseTargetOutputCapture(const AZ::CommandLine& cmd) { - const auto numSwitchValues = cmd.GetNumSwitchValues(OptionKeys[TargetOutputCapture]); - if (numSwitchValues) + if (const auto numSwitchValues = cmd.GetNumSwitchValues(OptionKeys[TargetOutputCapture]); + numSwitchValues) { AZ_TestImpact_Eval( numSwitchValues <= 2, CommandLineOptionsException, "Unexpected parameters for target output capture option"); @@ -253,8 +253,8 @@ namespace TestImpact AZStd::unordered_set ParseSuitesFilter(const AZ::CommandLine& cmd) { AZStd::unordered_set suitesFilter; - const auto numSwitchValues = cmd.GetNumSwitchValues(OptionKeys[SuitesFilter]); - if (numSwitchValues) + if (const auto numSwitchValues = cmd.GetNumSwitchValues(OptionKeys[SuitesFilter]); + numSwitchValues) { for (auto i = 0; i < numSwitchValues; i++) { @@ -460,4 +460,4 @@ namespace TestImpact return help; } -} +} // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h index 6958f03b80..73961b5fcc 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h @@ -25,7 +25,7 @@ namespace TestImpact //! The type of test sequence to run. enum class TestSequenceType { - None, //!< Runs no tests and will report a all tests successful. + None, //!< Runs no tests and will report all tests successful. Seed, //!< Removes any prior coverage data and runs all test targets with instrumentation to reseed the data from scratch. Regular, //!< Runs all of the test targets without any instrumentation to generate coverage data (any prior coverage data is left intact). ImpactAnalysis, //!< Uses any prior coverage data to run the instrumented subset of selected tests (if no prior coverage data a regular run is performed instead). @@ -108,4 +108,4 @@ namespace TestImpact AZStd::unordered_set m_suitesFilter; bool m_safeMode = false; }; -} +} // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp index 3315f68ae5..c03ba98303 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp @@ -12,13 +12,15 @@ #include +#include + namespace TestImpact { //! Attempts to parse a path option value. AZStd::optional ParsePathOption(const AZStd::string& optionName, const AZ::CommandLine& cmd) - { - const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); - if (numSwitchValues) + { + if (const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); + numSwitchValues) { AZ_TestImpact_Eval( numSwitchValues == 1, @@ -40,8 +42,8 @@ namespace TestImpact //! Attempts to pass an unsigned integer option value. AZStd::optional ParseUnsignedIntegerOption(const AZStd::string& optionName, const AZ::CommandLine& cmd) { - const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); - if (numSwitchValues) + if (const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); + numSwitchValues) { AZ_TestImpact_Eval( numSwitchValues == 1, @@ -49,11 +51,11 @@ namespace TestImpact AZStd::string::format("Unexpected number of parameters for %s option", optionName.c_str())); const auto str = cmd.GetSwitchValue(optionName, 0); - char* end = nullptr; - auto value = strtoul(str.c_str(), &end, 0); + size_t end = 0; + auto value = AZStd::stoul(str, &end, 0); AZ_TestImpact_Eval( - end != str, + end, CommandLineOptionsException, AZStd::string::format("Couldn't parse unsigned integer option value: %s", str.c_str())); @@ -74,4 +76,4 @@ namespace TestImpact return AZStd::nullopt; } -} +} // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h index 0da7f1f16e..e635d05710 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h @@ -38,8 +38,8 @@ namespace TestImpact const AZStd::pair, OptionValue>& state, const AZ::CommandLine& cmd) { - const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); - if (numSwitchValues) + if (const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); + numSwitchValues) { AZ_TestImpact_Eval( numSwitchValues == 1, @@ -70,8 +70,8 @@ namespace TestImpact const AZStd::vector>& states, const AZ::CommandLine& cmd) { - const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); - if (numSwitchValues) + if (const auto numSwitchValues = cmd.GetNumSwitchValues(optionName); + numSwitchValues) { AZ_TestImpact_Eval( numSwitchValues == 1, @@ -116,4 +116,4 @@ namespace TestImpact //! Attempts to parse an option value in seconds. AZStd::optional ParseSecondsOption(const AZStd::string& optionName, const AZ::CommandLine& cmd); -} +} // namespace TestImpact From c63de35b688aa4479133b8f456339148cd4476b3 Mon Sep 17 00:00:00 2001 From: jonawals Date: Fri, 28 May 2021 15:39:28 +0100 Subject: [PATCH 2/2] Address PR feedback --- .../Source/TestImpactCommandLineOptionsUtils.cpp | 10 +++++----- .../Source/TestImpactCommandLineOptionsUtils.h | 15 +++++++++------ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp index c03ba98303..b96716059f 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.cpp @@ -50,14 +50,14 @@ namespace TestImpact CommandLineOptionsException, AZStd::string::format("Unexpected number of parameters for %s option", optionName.c_str())); - const auto str = cmd.GetSwitchValue(optionName, 0); - size_t end = 0; - auto value = AZStd::stoul(str, &end, 0); + const auto strValue = cmd.GetSwitchValue(optionName, 0); + size_t successfulParse = 0; // Will be non-zero if the parse was successful + auto value = AZStd::stoul(strValue, &successfulParse, 0); AZ_TestImpact_Eval( - end, + successfulParse, CommandLineOptionsException, - AZStd::string::format("Couldn't parse unsigned integer option value: %s", str.c_str())); + AZStd::string::format("Couldn't parse unsigned integer option value: %s", strValue.c_str())); return aznumeric_caster(value); } diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h index e635d05710..1708efadd9 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptionsUtils.h @@ -47,13 +47,15 @@ namespace TestImpact AZStd::string::format("Unexpected number of parameters for %s option", optionName.c_str())); const auto option = cmd.GetSwitchValue(optionName, 0); - if (option == state.first.first) + if (const auto& [optionValueText, optionValue] = state.first; + option == optionValueText) { - return state.first.second; + return optionValue; } - else if (option == state.second.first) + if (const auto& [optionValueText, optionValue] = state.second; + option == optionValueText) { - return state.second.second; + return optionValue; } throw CommandLineOptionsException( @@ -81,9 +83,10 @@ namespace TestImpact const auto option = cmd.GetSwitchValue(optionName, 0); for (const auto& state : states) { - if (option == state.first) + if (const auto& [optionValueText, optionValue] = state; + option == optionValueText) { - return state.second; + return optionValue; } }