From bebb3a1f5ac05cf8ec9b7702019e700ed14887e3 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 03:29:00 +0300 Subject: Refactor: Use anonymous namespace instead of `static` keyword for consts Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 66 +++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index f99592c..9936d4c 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -22,38 +22,40 @@ class cmTest; -static std::string const keyAND = "AND"; -static std::string const keyCOMMAND = "COMMAND"; -static std::string const keyDEFINED = "DEFINED"; -static std::string const keyEQUAL = "EQUAL"; -static std::string const keyEXISTS = "EXISTS"; -static std::string const keyGREATER = "GREATER"; -static std::string const keyGREATER_EQUAL = "GREATER_EQUAL"; -static std::string const keyIN_LIST = "IN_LIST"; -static std::string const keyIS_ABSOLUTE = "IS_ABSOLUTE"; -static std::string const keyIS_DIRECTORY = "IS_DIRECTORY"; -static std::string const keyIS_NEWER_THAN = "IS_NEWER_THAN"; -static std::string const keyIS_SYMLINK = "IS_SYMLINK"; -static std::string const keyLESS = "LESS"; -static std::string const keyLESS_EQUAL = "LESS_EQUAL"; -static std::string const keyMATCHES = "MATCHES"; -static std::string const keyNOT = "NOT"; -static std::string const keyOR = "OR"; -static std::string const keyParenL = "("; -static std::string const keyParenR = ")"; -static std::string const keyPOLICY = "POLICY"; -static std::string const keySTREQUAL = "STREQUAL"; -static std::string const keySTRGREATER = "STRGREATER"; -static std::string const keySTRGREATER_EQUAL = "STRGREATER_EQUAL"; -static std::string const keySTRLESS = "STRLESS"; -static std::string const keySTRLESS_EQUAL = "STRLESS_EQUAL"; -static std::string const keyTARGET = "TARGET"; -static std::string const keyTEST = "TEST"; -static std::string const keyVERSION_EQUAL = "VERSION_EQUAL"; -static std::string const keyVERSION_GREATER = "VERSION_GREATER"; -static std::string const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"; -static std::string const keyVERSION_LESS = "VERSION_LESS"; -static std::string const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"; +namespace { +std::string const keyAND = "AND"; +std::string const keyCOMMAND = "COMMAND"; +std::string const keyDEFINED = "DEFINED"; +std::string const keyEQUAL = "EQUAL"; +std::string const keyEXISTS = "EXISTS"; +std::string const keyGREATER = "GREATER"; +std::string const keyGREATER_EQUAL = "GREATER_EQUAL"; +std::string const keyIN_LIST = "IN_LIST"; +std::string const keyIS_ABSOLUTE = "IS_ABSOLUTE"; +std::string const keyIS_DIRECTORY = "IS_DIRECTORY"; +std::string const keyIS_NEWER_THAN = "IS_NEWER_THAN"; +std::string const keyIS_SYMLINK = "IS_SYMLINK"; +std::string const keyLESS = "LESS"; +std::string const keyLESS_EQUAL = "LESS_EQUAL"; +std::string const keyMATCHES = "MATCHES"; +std::string const keyNOT = "NOT"; +std::string const keyOR = "OR"; +std::string const keyParenL = "("; +std::string const keyParenR = ")"; +std::string const keyPOLICY = "POLICY"; +std::string const keySTREQUAL = "STREQUAL"; +std::string const keySTRGREATER = "STRGREATER"; +std::string const keySTRGREATER_EQUAL = "STRGREATER_EQUAL"; +std::string const keySTRLESS = "STRLESS"; +std::string const keySTRLESS_EQUAL = "STRLESS_EQUAL"; +std::string const keyTARGET = "TARGET"; +std::string const keyTEST = "TEST"; +std::string const keyVERSION_EQUAL = "VERSION_EQUAL"; +std::string const keyVERSION_GREATER = "VERSION_GREATER"; +std::string const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"; +std::string const keyVERSION_LESS = "VERSION_LESS"; +std::string const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"; +} // anonymous namespace cmConditionEvaluator::cmConditionEvaluator(cmMakefile& makefile, cmListFileBacktrace bt) -- cgit v0.12 From 4b4e603075d61b33fac595ed29ac6dd694927927 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 03:34:08 +0300 Subject: Refactor: Add constness Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 9936d4c..530f8bc 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -229,7 +229,7 @@ bool cmConditionEvaluator::GetBooleanValue( // Check for numbers. if (!arg.empty()) { char* end; - double d = strtod(arg.GetValue().c_str(), &end); + const double d = std::strtod(arg.GetValue().c_str(), &end); if (*end == '\0') { // The whole string is a number. Use C conversion to bool. return static_cast(d); @@ -280,8 +280,8 @@ bool cmConditionEvaluator::GetBooleanValueWithAutoDereference( } // Check policy only if old and new results differ. - bool newResult = this->GetBooleanValue(newArg); - bool oldResult = this->GetBooleanValueOld(newArg, oneArg); + const bool newResult = this->GetBooleanValue(newArg); + const bool oldResult = this->GetBooleanValueOld(newArg, oneArg); if (newResult != oldResult) { switch (this->Policy12Status) { case cmPolicies::WARN: @@ -323,7 +323,7 @@ void cmConditionEvaluator::IncrementArguments( //========================================================================= // helper function to reduce code duplication void cmConditionEvaluator::HandlePredicate( - bool value, int& reducible, cmArgumentList::iterator& arg, + const bool value, int& reducible, cmArgumentList::iterator& arg, cmArgumentList& newArgs, cmArgumentList::iterator& argP1, cmArgumentList::iterator& argP2) const { @@ -340,7 +340,7 @@ void cmConditionEvaluator::HandlePredicate( //========================================================================= // helper function to reduce code duplication -void cmConditionEvaluator::HandleBinaryOp(bool value, int& reducible, +void cmConditionEvaluator::HandleBinaryOp(const bool value, int& reducible, cmArgumentList::iterator& arg, cmArgumentList& newArgs, cmArgumentList::iterator& argP1, @@ -491,7 +491,7 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } // is a variable defined if (this->IsKeyword(keyDEFINED, *arg) && argP1 != newArgs.end()) { - size_t argP1len = argP1->GetValue().size(); + const size_t argP1len = argP1->GetValue().size(); bool bdef = false; if (argP1len > 4 && cmHasLiteralPrefix(argP1->GetValue(), "ENV{") && argP1->GetValue().operator[](argP1len - 1) == '}') { @@ -585,8 +585,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, double lhs; double rhs; bool result; - if (sscanf(def->c_str(), "%lg", &lhs) != 1 || - sscanf(def2->c_str(), "%lg", &rhs) != 1) { + if (std::sscanf(def->c_str(), "%lg", &lhs) != 1 || + std::sscanf(def2->c_str(), "%lg", &rhs) != 1) { result = false; } else if (*(argP1) == keyLESS) { result = (lhs < rhs); @@ -610,7 +610,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keySTREQUAL, *argP1))) { def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); - int val = (*def).compare(*def2); + const int val = (*def).compare(*def2); bool result; if (*(argP1) == keySTRLESS) { result = (val < 0); @@ -647,7 +647,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } else { // version_equal op = cmSystemTools::OP_EQUAL; } - bool result = + const bool result = cmSystemTools::VersionCompare(op, def->c_str(), def2->c_str()); this->HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } -- cgit v0.12 From fab389002543f9a7c46522956dd8ce6bfe1752a4 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 03:36:44 +0300 Subject: Refactor: Opt-out `if` stataments to select 1st param ... for some calls to `cmExpandedCommandArgument`. Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 530f8bc..4a9f95f 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -2,6 +2,7 @@ file Copyright.txt or https://cmake.org/licensing for details. */ #include "cmConditionEvaluator.h" +#include #include #include #include @@ -55,6 +56,9 @@ std::string const keyVERSION_GREATER = "VERSION_GREATER"; std::string const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"; std::string const keyVERSION_LESS = "VERSION_LESS"; std::string const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"; + +std::array const ZERO_ONE_XLAT = { "0", "1" }; + } // anonymous namespace cmConditionEvaluator::cmConditionEvaluator(cmMakefile& makefile, @@ -312,10 +316,10 @@ void cmConditionEvaluator::IncrementArguments( cmArgumentList::iterator& argP2) const { if (argP1 != newArgs.end()) { - argP1++; + ++argP1; argP2 = argP1; if (argP1 != newArgs.end()) { - argP2++; + ++argP2; } } } @@ -327,11 +331,7 @@ void cmConditionEvaluator::HandlePredicate( cmArgumentList& newArgs, cmArgumentList::iterator& argP1, cmArgumentList::iterator& argP2) const { - if (value) { - *arg = cmExpandedCommandArgument("1", true); - } else { - *arg = cmExpandedCommandArgument("0", true); - } + *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); newArgs.erase(argP1); argP1 = arg; this->IncrementArguments(newArgs, argP1, argP2); @@ -346,11 +346,7 @@ void cmConditionEvaluator::HandleBinaryOp(const bool value, int& reducible, cmArgumentList::iterator& argP1, cmArgumentList::iterator& argP2) { - if (value) { - *arg = cmExpandedCommandArgument("1", true); - } else { - *arg = cmExpandedCommandArgument("0", true); - } + *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); newArgs.erase(argP2); newArgs.erase(argP1); argP1 = arg; @@ -399,12 +395,8 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, newArgs2.pop_back(); // now recursively invoke IsTrue to handle the values inside the // parenthetical expression - bool value = this->IsTrue(newArgs2, errorString, status); - if (value) { - *arg = cmExpandedCommandArgument("1", true); - } else { - *arg = cmExpandedCommandArgument("0", true); - } + const bool value = this->IsTrue(newArgs2, errorString, status); + *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); argP1 = arg; argP1++; // remove the now evaluated parenthetical expression -- cgit v0.12 From c26f15c66dcf45c2245315251b31b8f0b671321b Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 04:38:40 +0300 Subject: Refactor: Use `cm::string_view` for static const literals Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 93 +++++++++++++++++++++-------------------- Source/cmConditionEvaluator.h | 4 +- 2 files changed, 50 insertions(+), 47 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 4a9f95f..29d7108 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -9,6 +9,7 @@ #include #include +#include #include #include "cmsys/RegularExpression.hxx" @@ -24,38 +25,38 @@ class cmTest; namespace { -std::string const keyAND = "AND"; -std::string const keyCOMMAND = "COMMAND"; -std::string const keyDEFINED = "DEFINED"; -std::string const keyEQUAL = "EQUAL"; -std::string const keyEXISTS = "EXISTS"; -std::string const keyGREATER = "GREATER"; -std::string const keyGREATER_EQUAL = "GREATER_EQUAL"; -std::string const keyIN_LIST = "IN_LIST"; -std::string const keyIS_ABSOLUTE = "IS_ABSOLUTE"; -std::string const keyIS_DIRECTORY = "IS_DIRECTORY"; -std::string const keyIS_NEWER_THAN = "IS_NEWER_THAN"; -std::string const keyIS_SYMLINK = "IS_SYMLINK"; -std::string const keyLESS = "LESS"; -std::string const keyLESS_EQUAL = "LESS_EQUAL"; -std::string const keyMATCHES = "MATCHES"; -std::string const keyNOT = "NOT"; -std::string const keyOR = "OR"; -std::string const keyParenL = "("; -std::string const keyParenR = ")"; -std::string const keyPOLICY = "POLICY"; -std::string const keySTREQUAL = "STREQUAL"; -std::string const keySTRGREATER = "STRGREATER"; -std::string const keySTRGREATER_EQUAL = "STRGREATER_EQUAL"; -std::string const keySTRLESS = "STRLESS"; -std::string const keySTRLESS_EQUAL = "STRLESS_EQUAL"; -std::string const keyTARGET = "TARGET"; -std::string const keyTEST = "TEST"; -std::string const keyVERSION_EQUAL = "VERSION_EQUAL"; -std::string const keyVERSION_GREATER = "VERSION_GREATER"; -std::string const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"; -std::string const keyVERSION_LESS = "VERSION_LESS"; -std::string const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"; +auto const keyAND = "AND"_s; +auto const keyCOMMAND = "COMMAND"_s; +auto const keyDEFINED = "DEFINED"_s; +auto const keyEQUAL = "EQUAL"_s; +auto const keyEXISTS = "EXISTS"_s; +auto const keyGREATER = "GREATER"_s; +auto const keyGREATER_EQUAL = "GREATER_EQUAL"_s; +auto const keyIN_LIST = "IN_LIST"_s; +auto const keyIS_ABSOLUTE = "IS_ABSOLUTE"_s; +auto const keyIS_DIRECTORY = "IS_DIRECTORY"_s; +auto const keyIS_NEWER_THAN = "IS_NEWER_THAN"_s; +auto const keyIS_SYMLINK = "IS_SYMLINK"_s; +auto const keyLESS = "LESS"_s; +auto const keyLESS_EQUAL = "LESS_EQUAL"_s; +auto const keyMATCHES = "MATCHES"_s; +auto const keyNOT = "NOT"_s; +auto const keyOR = "OR"_s; +auto const keyParenL = "("_s; +auto const keyParenR = ")"_s; +auto const keyPOLICY = "POLICY"_s; +auto const keySTREQUAL = "STREQUAL"_s; +auto const keySTRGREATER = "STRGREATER"_s; +auto const keySTRGREATER_EQUAL = "STRGREATER_EQUAL"_s; +auto const keySTRLESS = "STRLESS"_s; +auto const keySTRLESS_EQUAL = "STRLESS_EQUAL"_s; +auto const keyTARGET = "TARGET"_s; +auto const keyTEST = "TEST"_s; +auto const keyVERSION_EQUAL = "VERSION_EQUAL"_s; +auto const keyVERSION_GREATER = "VERSION_GREATER"_s; +auto const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"_s; +auto const keyVERSION_LESS = "VERSION_LESS"_s; +auto const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"_s; std::array const ZERO_ONE_XLAT = { "0", "1" }; @@ -181,7 +182,7 @@ cmProp cmConditionEvaluator::GetVariableOrString( } //========================================================================= -bool cmConditionEvaluator::IsKeyword(std::string const& keyword, +bool cmConditionEvaluator::IsKeyword(cm::string_view keyword, cmExpandedCommandArgument& argument) const { if ((this->Policy54Status != cmPolicies::WARN && @@ -190,7 +191,7 @@ bool cmConditionEvaluator::IsKeyword(std::string const& keyword, return false; } - bool isKeyword = argument.GetValue() == keyword; + const bool isKeyword = argument.GetValue() == keyword; if (isKeyword && argument.WasQuoted() && this->Policy54Status == cmPolicies::WARN) { @@ -580,13 +581,13 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, if (std::sscanf(def->c_str(), "%lg", &lhs) != 1 || std::sscanf(def2->c_str(), "%lg", &rhs) != 1) { result = false; - } else if (*(argP1) == keyLESS) { + } else if (argP1->GetValue() == keyLESS) { result = (lhs < rhs); - } else if (*(argP1) == keyLESS_EQUAL) { + } else if (argP1->GetValue() == keyLESS_EQUAL) { result = (lhs <= rhs); - } else if (*(argP1) == keyGREATER) { + } else if (argP1->GetValue() == keyGREATER) { result = (lhs > rhs); - } else if (*(argP1) == keyGREATER_EQUAL) { + } else if (argP1->GetValue() == keyGREATER_EQUAL) { result = (lhs >= rhs); } else { result = (lhs == rhs); @@ -604,13 +605,13 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, def2 = this->GetVariableOrString(*argP2); const int val = (*def).compare(*def2); bool result; - if (*(argP1) == keySTRLESS) { + if (argP1->GetValue() == keySTRLESS) { result = (val < 0); - } else if (*(argP1) == keySTRLESS_EQUAL) { + } else if (argP1->GetValue() == keySTRLESS_EQUAL) { result = (val <= 0); - } else if (*(argP1) == keySTRGREATER) { + } else if (argP1->GetValue() == keySTRGREATER) { result = (val > 0); - } else if (*(argP1) == keySTRGREATER_EQUAL) { + } else if (argP1->GetValue() == keySTRGREATER_EQUAL) { result = (val >= 0); } else // strequal { @@ -628,13 +629,13 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); cmSystemTools::CompareOp op; - if (*argP1 == keyVERSION_LESS) { + if (argP1->GetValue() == keyVERSION_LESS) { op = cmSystemTools::OP_LESS; - } else if (*argP1 == keyVERSION_LESS_EQUAL) { + } else if (argP1->GetValue() == keyVERSION_LESS_EQUAL) { op = cmSystemTools::OP_LESS_EQUAL; - } else if (*argP1 == keyVERSION_GREATER) { + } else if (argP1->GetValue() == keyVERSION_GREATER) { op = cmSystemTools::OP_GREATER; - } else if (*argP1 == keyVERSION_GREATER_EQUAL) { + } else if (argP1->GetValue() == keyVERSION_GREATER_EQUAL) { op = cmSystemTools::OP_GREATER_EQUAL; } else { // version_equal op = cmSystemTools::OP_EQUAL; diff --git a/Source/cmConditionEvaluator.h b/Source/cmConditionEvaluator.h index cf00ede..ce25519 100644 --- a/Source/cmConditionEvaluator.h +++ b/Source/cmConditionEvaluator.h @@ -8,6 +8,8 @@ #include #include +#include + #include "cmExpandedCommandArgument.h" #include "cmListFileCache.h" #include "cmMessageType.h" @@ -36,7 +38,7 @@ private: cmProp GetVariableOrString(const cmExpandedCommandArgument& argument) const; - bool IsKeyword(std::string const& keyword, + bool IsKeyword(cm::string_view keyword, cmExpandedCommandArgument& argument) const; bool GetBooleanValue(cmExpandedCommandArgument& arg) const; -- cgit v0.12 From 24cbfb8d964564711c764658a295abba560e837a Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 20:30:12 +0300 Subject: Refactor: Turn `reducible` flag into boolean Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 32 ++++++++++++++++---------------- Source/cmConditionEvaluator.h | 4 ++-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 29d7108..a6e8147 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -328,7 +328,7 @@ void cmConditionEvaluator::IncrementArguments( //========================================================================= // helper function to reduce code duplication void cmConditionEvaluator::HandlePredicate( - const bool value, int& reducible, cmArgumentList::iterator& arg, + const bool value, bool& reducible, cmArgumentList::iterator& arg, cmArgumentList& newArgs, cmArgumentList::iterator& argP1, cmArgumentList::iterator& argP2) const { @@ -336,12 +336,12 @@ void cmConditionEvaluator::HandlePredicate( newArgs.erase(argP1); argP1 = arg; this->IncrementArguments(newArgs, argP1, argP2); - reducible = 1; + reducible = true; } //========================================================================= // helper function to reduce code duplication -void cmConditionEvaluator::HandleBinaryOp(const bool value, int& reducible, +void cmConditionEvaluator::HandleBinaryOp(const bool value, bool& reducible, cmArgumentList::iterator& arg, cmArgumentList& newArgs, cmArgumentList::iterator& argP1, @@ -352,7 +352,7 @@ void cmConditionEvaluator::HandleBinaryOp(const bool value, int& reducible, newArgs.erase(argP1); argP1 = arg; this->IncrementArguments(newArgs, argP1, argP2); - reducible = 1; + reducible = true; } //========================================================================= @@ -361,9 +361,9 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - int reducible; + bool reducible; do { - reducible = 0; + reducible = false; auto arg = newArgs.begin(); while (arg != newArgs.end()) { if (this->IsKeyword(keyParenL, *arg)) { @@ -414,9 +414,9 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, MessageType&) { - int reducible; + bool reducible; do { - reducible = 0; + reducible = false; auto arg = newArgs.begin(); cmArgumentList::iterator argP1; cmArgumentList::iterator argP2; @@ -513,12 +513,12 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - int reducible; + bool reducible; std::string def_buf; cmProp def; cmProp def2; do { - reducible = 0; + reducible = false; auto arg = newArgs.begin(); cmArgumentList::iterator argP1; cmArgumentList::iterator argP2; @@ -556,7 +556,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, newArgs.erase(argP1); argP1 = arg; this->IncrementArguments(newArgs, argP1, argP2); - reducible = 1; + reducible = true; } if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { @@ -564,7 +564,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, newArgs.erase(argP1); argP1 = arg; this->IncrementArguments(newArgs, argP1, argP2); - reducible = 1; + reducible = true; } if (argP1 != newArgs.end() && argP2 != newArgs.end() && @@ -694,9 +694,9 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - int reducible; + bool reducible; do { - reducible = 0; + reducible = false; auto arg = newArgs.begin(); cmArgumentList::iterator argP1; cmArgumentList::iterator argP2; @@ -720,11 +720,11 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - int reducible; + bool reducible; bool lhs; bool rhs; do { - reducible = 0; + reducible = false; auto arg = newArgs.begin(); cmArgumentList::iterator argP1; cmArgumentList::iterator argP2; diff --git a/Source/cmConditionEvaluator.h b/Source/cmConditionEvaluator.h index ce25519..5d48ee8 100644 --- a/Source/cmConditionEvaluator.h +++ b/Source/cmConditionEvaluator.h @@ -55,12 +55,12 @@ private: cmArgumentList::iterator& argP1, cmArgumentList::iterator& argP2) const; - void HandlePredicate(bool value, int& reducible, + void HandlePredicate(bool value, bool& reducible, cmArgumentList::iterator& arg, cmArgumentList& newArgs, cmArgumentList::iterator& argP1, cmArgumentList::iterator& argP2) const; - void HandleBinaryOp(bool value, int& reducible, + void HandleBinaryOp(bool value, bool& reducible, cmArgumentList::iterator& arg, cmArgumentList& newArgs, cmArgumentList::iterator& argP1, cmArgumentList::iterator& argP2); -- cgit v0.12 From 0f65d0cd837c2c12f04e8e10cec6c49eade08f1d Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 20:54:29 +0300 Subject: Refactor: Turn the innter `while` loop in `HandleLevelN()` into `for` Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index a6e8147..ef51729 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -317,8 +317,7 @@ void cmConditionEvaluator::IncrementArguments( cmArgumentList::iterator& argP2) const { if (argP1 != newArgs.end()) { - ++argP1; - argP2 = argP1; + argP2 = ++argP1; if (argP1 != newArgs.end()) { ++argP2; } @@ -364,8 +363,7 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, bool reducible; do { reducible = false; - auto arg = newArgs.begin(); - while (arg != newArgs.end()) { + for (auto arg = newArgs.begin(); arg != newArgs.end(); ++arg) { if (this->IsKeyword(keyParenL, *arg)) { // search for the closing paren for this opening one cmArgumentList::iterator argClose; @@ -403,7 +401,6 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, // remove the now evaluated parenthetical expression newArgs.erase(argP1, argClose); } - ++arg; } } while (reducible); return true; @@ -417,11 +414,8 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, bool reducible; do { reducible = false; - auto arg = newArgs.begin(); - cmArgumentList::iterator argP1; - cmArgumentList::iterator argP2; - while (arg != newArgs.end()) { - argP1 = arg; + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; + arg != newArgs.end(); argP1 = ++arg) { this->IncrementArguments(newArgs, argP1, argP2); // does a file exist if (this->IsKeyword(keyEXISTS, *arg) && argP1 != newArgs.end()) { @@ -501,7 +495,6 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } this->HandlePredicate(bdef, reducible, arg, newArgs, argP1, argP2); } - ++arg; } } while (reducible); return true; @@ -519,11 +512,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, cmProp def2; do { reducible = false; - auto arg = newArgs.begin(); - cmArgumentList::iterator argP1; - cmArgumentList::iterator argP2; - while (arg != newArgs.end()) { - argP1 = arg; + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; + arg != newArgs.end(); argP1 = ++arg) { this->IncrementArguments(newArgs, argP1, argP2); if (argP1 != newArgs.end() && argP2 != newArgs.end() && this->IsKeyword(keyMATCHES, *argP1)) { @@ -681,8 +671,6 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); } } - - ++arg; } } while (reducible); return true; @@ -697,18 +685,14 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, bool reducible; do { reducible = false; - auto arg = newArgs.begin(); - cmArgumentList::iterator argP1; - cmArgumentList::iterator argP2; - while (arg != newArgs.end()) { - argP1 = arg; + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; + arg != newArgs.end(); argP1 = ++arg) { this->IncrementArguments(newArgs, argP1, argP2); if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { bool rhs = this->GetBooleanValueWithAutoDereference( *argP1, errorString, status); this->HandlePredicate(!rhs, reducible, arg, newArgs, argP1, argP2); } - ++arg; } } while (reducible); return true; @@ -725,11 +709,8 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, bool rhs; do { reducible = false; - auto arg = newArgs.begin(); - cmArgumentList::iterator argP1; - cmArgumentList::iterator argP2; - while (arg != newArgs.end()) { - argP1 = arg; + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; + arg != newArgs.end(); argP1 = ++arg) { this->IncrementArguments(newArgs, argP1, argP2); if (argP1 != newArgs.end() && this->IsKeyword(keyAND, *argP1) && argP2 != newArgs.end()) { @@ -750,7 +731,6 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, this->HandleBinaryOp((lhs || rhs), reducible, arg, newArgs, argP1, argP2); } - ++arg; } } while (reducible); return true; -- cgit v0.12 From 961df6cde60763dc77cafe6bf3a2704b52394a20 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 21:10:06 +0300 Subject: Refactor: Make `IncrementArguments()` the free function Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 43 +++++++++++++++++++++-------------------- Source/cmConditionEvaluator.h | 4 ---- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index ef51729..50e7655 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -60,6 +61,19 @@ auto const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"_s; std::array const ZERO_ONE_XLAT = { "0", "1" }; +inline void IncrementArguments( + cmConditionEvaluator::cmArgumentList& newArgs, + cmConditionEvaluator::cmArgumentList::iterator& argP1, + cmConditionEvaluator::cmArgumentList::iterator& argP2) +{ + if (argP1 != newArgs.end()) { + argP2 = ++argP1; + using difference_type = + cmConditionEvaluator::cmArgumentList::difference_type; + std::advance(argP2, difference_type(argP1 != newArgs.end())); + } +} + } // anonymous namespace cmConditionEvaluator::cmConditionEvaluator(cmMakefile& makefile, @@ -312,19 +326,6 @@ bool cmConditionEvaluator::GetBooleanValueWithAutoDereference( } //========================================================================= -void cmConditionEvaluator::IncrementArguments( - cmArgumentList& newArgs, cmArgumentList::iterator& argP1, - cmArgumentList::iterator& argP2) const -{ - if (argP1 != newArgs.end()) { - argP2 = ++argP1; - if (argP1 != newArgs.end()) { - ++argP2; - } - } -} - -//========================================================================= // helper function to reduce code duplication void cmConditionEvaluator::HandlePredicate( const bool value, bool& reducible, cmArgumentList::iterator& arg, @@ -334,7 +335,7 @@ void cmConditionEvaluator::HandlePredicate( *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); newArgs.erase(argP1); argP1 = arg; - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); reducible = true; } @@ -350,7 +351,7 @@ void cmConditionEvaluator::HandleBinaryOp(const bool value, bool& reducible, newArgs.erase(argP2); newArgs.erase(argP1); argP1 = arg; - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); reducible = true; } @@ -416,7 +417,7 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); // does a file exist if (this->IsKeyword(keyEXISTS, *arg) && argP1 != newArgs.end()) { this->HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), @@ -514,7 +515,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); if (argP1 != newArgs.end() && argP2 != newArgs.end() && this->IsKeyword(keyMATCHES, *argP1)) { def = this->GetDefinitionIfUnquoted(*arg); @@ -545,7 +546,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, newArgs.erase(argP2); newArgs.erase(argP1); argP1 = arg; - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); reducible = true; } @@ -553,7 +554,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, *arg = cmExpandedCommandArgument("0", true); newArgs.erase(argP1); argP1 = arg; - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); reducible = true; } @@ -687,7 +688,7 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { bool rhs = this->GetBooleanValueWithAutoDereference( *argP1, errorString, status); @@ -711,7 +712,7 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { - this->IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); if (argP1 != newArgs.end() && this->IsKeyword(keyAND, *argP1) && argP2 != newArgs.end()) { lhs = diff --git a/Source/cmConditionEvaluator.h b/Source/cmConditionEvaluator.h index 5d48ee8..74652a0 100644 --- a/Source/cmConditionEvaluator.h +++ b/Source/cmConditionEvaluator.h @@ -51,10 +51,6 @@ private: MessageType& status, bool oneArg = false) const; - void IncrementArguments(cmArgumentList& newArgs, - cmArgumentList::iterator& argP1, - cmArgumentList::iterator& argP2) const; - void HandlePredicate(bool value, bool& reducible, cmArgumentList::iterator& arg, cmArgumentList& newArgs, cmArgumentList::iterator& argP1, -- cgit v0.12 From 498c8c77732acf71e9a2f21afe37ee3f815b8801 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sat, 24 Jul 2021 22:45:09 +0300 Subject: Refactor: More `auto` and constness Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 60 +++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 50e7655..ad16804 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -23,8 +23,6 @@ #include "cmSystemTools.h" #include "cmake.h" -class cmTest; - namespace { auto const keyAND = "AND"_s; auto const keyCOMMAND = "COMMAND"_s; @@ -205,7 +203,7 @@ bool cmConditionEvaluator::IsKeyword(cm::string_view keyword, return false; } - const bool isKeyword = argument.GetValue() == keyword; + const auto isKeyword = argument.GetValue() == keyword; if (isKeyword && argument.WasQuoted() && this->Policy54Status == cmPolicies::WARN) { @@ -263,7 +261,7 @@ bool cmConditionEvaluator::GetBooleanValue( //========================================================================= // Boolean value behavior from CMake 2.6.4 and below. bool cmConditionEvaluator::GetBooleanValueOld( - cmExpandedCommandArgument const& arg, bool one) const + cmExpandedCommandArgument const& arg, bool const one) const { if (one) { // Old IsTrue behavior for single argument. @@ -278,7 +276,7 @@ bool cmConditionEvaluator::GetBooleanValueOld( } // Old GetVariableOrNumber behavior. cmProp def = this->GetDefinitionIfUnquoted(arg); - if (!def && atoi(arg.GetValue().c_str())) { + if (!def && std::atoi(arg.GetValue().c_str())) { def = &arg.GetValue(); } return !cmIsOff(def); @@ -288,7 +286,7 @@ bool cmConditionEvaluator::GetBooleanValueOld( // returns the resulting boolean value bool cmConditionEvaluator::GetBooleanValueWithAutoDereference( cmExpandedCommandArgument& newArg, std::string& errorString, - MessageType& status, bool oneArg) const + MessageType& status, bool const oneArg) const { // Use the policy if it is set. if (this->Policy12Status == cmPolicies::NEW) { @@ -299,8 +297,8 @@ bool cmConditionEvaluator::GetBooleanValueWithAutoDereference( } // Check policy only if old and new results differ. - const bool newResult = this->GetBooleanValue(newArg); - const bool oldResult = this->GetBooleanValueOld(newArg, oneArg); + const auto newResult = this->GetBooleanValue(newArg); + const auto oldResult = this->GetBooleanValueOld(newArg, oneArg); if (newResult != oldResult) { switch (this->Policy12Status) { case cmPolicies::WARN: @@ -370,7 +368,7 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, cmArgumentList::iterator argClose; argClose = arg; argClose++; - unsigned int depth = 1; + auto depth = 1u; while (argClose != newArgs.end() && depth) { if (this->IsKeyword(keyParenL, *argClose)) { depth++; @@ -389,16 +387,14 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, std::vector newArgs2; // copy to the list structure - auto argP1 = arg; - argP1++; + auto argP1 = std::next(arg); cm::append(newArgs2, argP1, argClose); newArgs2.pop_back(); // now recursively invoke IsTrue to handle the values inside the // parenthetical expression - const bool value = this->IsTrue(newArgs2, errorString, status); + const auto value = this->IsTrue(newArgs2, errorString, status); *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); - argP1 = arg; - argP1++; + argP1 = std::next(arg); // remove the now evaluated parenthetical expression newArgs.erase(argP1, argClose); } @@ -441,10 +437,9 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } // does a command exist if (this->IsKeyword(keyCOMMAND, *arg) && argP1 != newArgs.end()) { - cmState::Command command = - this->Makefile.GetState()->GetCommand(argP1->GetValue()); - this->HandlePredicate(command != nullptr, reducible, arg, newArgs, - argP1, argP2); + this->HandlePredicate( + this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, + reducible, arg, newArgs, argP1, argP2); } // does a policy exist if (this->IsKeyword(keyPOLICY, *arg) && argP1 != newArgs.end()) { @@ -463,9 +458,9 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, if (this->Policy64Status != cmPolicies::OLD && this->Policy64Status != cmPolicies::WARN) { if (this->IsKeyword(keyTEST, *arg) && argP1 != newArgs.end()) { - const cmTest* haveTest = this->Makefile.GetTest(argP1->GetValue()); - this->HandlePredicate(haveTest != nullptr, reducible, arg, newArgs, - argP1, argP2); + this->HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != + nullptr, + reducible, arg, newArgs, argP1, argP2); } } else if (this->Policy64Status == cmPolicies::WARN && this->IsKeyword(keyTEST, *arg)) { @@ -479,16 +474,16 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } // is a variable defined if (this->IsKeyword(keyDEFINED, *arg) && argP1 != newArgs.end()) { - const size_t argP1len = argP1->GetValue().size(); - bool bdef = false; + const auto argP1len = argP1->GetValue().size(); + auto bdef = false; if (argP1len > 4 && cmHasLiteralPrefix(argP1->GetValue(), "ENV{") && argP1->GetValue().operator[](argP1len - 1) == '}') { - std::string env = argP1->GetValue().substr(4, argP1len - 5); + const auto env = argP1->GetValue().substr(4, argP1len - 5); bdef = cmSystemTools::HasEnv(env); } else if (argP1len > 6 && cmHasLiteralPrefix(argP1->GetValue(), "CACHE{") && argP1->GetValue().operator[](argP1len - 1) == '}') { - std::string cache = argP1->GetValue().substr(6, argP1len - 7); + const auto cache = argP1->GetValue().substr(6, argP1len - 7); bdef = this->Makefile.GetState()->GetCacheEntryValue(cache) != nullptr; } else { @@ -527,7 +522,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, def_buf = *def; def = &def_buf; } - const std::string& rex = argP2->GetValue(); + const auto& rex = argP2->GetValue(); this->Makefile.ClearMatches(); cmsys::RegularExpression regEntry; if (!regEntry.compile(rex)) { @@ -631,7 +626,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } else { // version_equal op = cmSystemTools::OP_EQUAL; } - const bool result = + const auto result = cmSystemTools::VersionCompare(op, def->c_str(), def2->c_str()); this->HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } @@ -639,9 +634,9 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, // is file A newer than file B if (argP1 != newArgs.end() && argP2 != newArgs.end() && this->IsKeyword(keyIS_NEWER_THAN, *argP1)) { - int fileIsNewer = 0; + auto fileIsNewer = 0; cmsys::Status ftcStatus = cmSystemTools::FileTimeCompare( - arg->GetValue(), (argP2)->GetValue(), &fileIsNewer); + arg->GetValue(), argP2->GetValue(), &fileIsNewer); this->HandleBinaryOp( (!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), reducible, arg, newArgs, argP1, argP2); @@ -651,14 +646,13 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keyIN_LIST, *argP1)) { if (this->Policy57Status != cmPolicies::OLD && this->Policy57Status != cmPolicies::WARN) { - bool result = false; + auto result = false; def = this->GetVariableOrString(*arg); def2 = this->Makefile.GetDefinition(argP2->GetValue()); if (def2) { - std::vector list = cmExpandedList(*def2, true); - result = cm::contains(list, *def); + result = cm::contains(cmExpandedList(*def2, true), *def); } this->HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); @@ -690,7 +684,7 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, arg != newArgs.end(); argP1 = ++arg) { IncrementArguments(newArgs, argP1, argP2); if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { - bool rhs = this->GetBooleanValueWithAutoDereference( + const auto rhs = this->GetBooleanValueWithAutoDereference( *argP1, errorString, status); this->HandlePredicate(!rhs, reducible, arg, newArgs, argP1, argP2); } -- cgit v0.12 From 135c37bdd73a8de5a2da43e652f26948481dd936 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 21:27:56 +0300 Subject: Refactor: Make `HandleBinaryOp` and `HandlePredicate` free functions --- Source/cmConditionEvaluator.cxx | 112 +++++++++++++++++++--------------------- Source/cmConditionEvaluator.h | 10 ---- 2 files changed, 52 insertions(+), 70 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index ad16804..be7ae88 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -72,6 +72,33 @@ inline void IncrementArguments( } } +void HandlePredicate(const bool value, bool& reducible, + cmConditionEvaluator::cmArgumentList::iterator& arg, + cmConditionEvaluator::cmArgumentList& newArgs, + cmConditionEvaluator::cmArgumentList::iterator& argP1, + cmConditionEvaluator::cmArgumentList::iterator& argP2) +{ + *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); + newArgs.erase(argP1); + argP1 = arg; + IncrementArguments(newArgs, argP1, argP2); + reducible = true; +} + +void HandleBinaryOp(const bool value, bool& reducible, + cmConditionEvaluator::cmArgumentList::iterator& arg, + cmConditionEvaluator::cmArgumentList& newArgs, + cmConditionEvaluator::cmArgumentList::iterator& argP1, + cmConditionEvaluator::cmArgumentList::iterator& argP2) +{ + *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); + newArgs.erase(argP2); + newArgs.erase(argP1); + argP1 = arg; + IncrementArguments(newArgs, argP1, argP2); + reducible = true; +} + } // anonymous namespace cmConditionEvaluator::cmConditionEvaluator(cmMakefile& makefile, @@ -324,36 +351,6 @@ bool cmConditionEvaluator::GetBooleanValueWithAutoDereference( } //========================================================================= -// helper function to reduce code duplication -void cmConditionEvaluator::HandlePredicate( - const bool value, bool& reducible, cmArgumentList::iterator& arg, - cmArgumentList& newArgs, cmArgumentList::iterator& argP1, - cmArgumentList::iterator& argP2) const -{ - *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); - newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); - reducible = true; -} - -//========================================================================= -// helper function to reduce code duplication -void cmConditionEvaluator::HandleBinaryOp(const bool value, bool& reducible, - cmArgumentList::iterator& arg, - cmArgumentList& newArgs, - cmArgumentList::iterator& argP1, - cmArgumentList::iterator& argP2) -{ - *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); - newArgs.erase(argP2); - newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); - reducible = true; -} - -//========================================================================= // level 0 processes parenthetical expressions bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, std::string& errorString, @@ -416,51 +413,49 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, IncrementArguments(newArgs, argP1, argP2); // does a file exist if (this->IsKeyword(keyEXISTS, *arg) && argP1 != newArgs.end()) { - this->HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), - reducible, arg, newArgs, argP1, argP2); + HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), + reducible, arg, newArgs, argP1, argP2); } // does a directory with this name exist if (this->IsKeyword(keyIS_DIRECTORY, *arg) && argP1 != newArgs.end()) { - this->HandlePredicate( - cmSystemTools::FileIsDirectory(argP1->GetValue()), reducible, arg, - newArgs, argP1, argP2); + HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), + reducible, arg, newArgs, argP1, argP2); } // does a symlink with this name exist if (this->IsKeyword(keyIS_SYMLINK, *arg) && argP1 != newArgs.end()) { - this->HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), - reducible, arg, newArgs, argP1, argP2); + HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), + reducible, arg, newArgs, argP1, argP2); } // is the given path an absolute path ? if (this->IsKeyword(keyIS_ABSOLUTE, *arg) && argP1 != newArgs.end()) { - this->HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), - reducible, arg, newArgs, argP1, argP2); + HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), + reducible, arg, newArgs, argP1, argP2); } // does a command exist if (this->IsKeyword(keyCOMMAND, *arg) && argP1 != newArgs.end()) { - this->HandlePredicate( + HandlePredicate( this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, reducible, arg, newArgs, argP1, argP2); } // does a policy exist if (this->IsKeyword(keyPOLICY, *arg) && argP1 != newArgs.end()) { cmPolicies::PolicyID pid; - this->HandlePredicate( + HandlePredicate( cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), reducible, arg, newArgs, argP1, argP2); } // does a target exist if (this->IsKeyword(keyTARGET, *arg) && argP1 != newArgs.end()) { - this->HandlePredicate( - this->Makefile.FindTargetToUse(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1, argP2); + HandlePredicate(this->Makefile.FindTargetToUse(argP1->GetValue()) != + nullptr, + reducible, arg, newArgs, argP1, argP2); } // does a test exist if (this->Policy64Status != cmPolicies::OLD && this->Policy64Status != cmPolicies::WARN) { if (this->IsKeyword(keyTEST, *arg) && argP1 != newArgs.end()) { - this->HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != - nullptr, - reducible, arg, newArgs, argP1, argP2); + HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, + reducible, arg, newArgs, argP1, argP2); } } else if (this->Policy64Status == cmPolicies::WARN && this->IsKeyword(keyTEST, *arg)) { @@ -489,7 +484,7 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } else { bdef = this->Makefile.IsDefinitionSet(argP1->GetValue()); } - this->HandlePredicate(bdef, reducible, arg, newArgs, argP1, argP2); + HandlePredicate(bdef, reducible, arg, newArgs, argP1, argP2); } } } while (reducible); @@ -578,7 +573,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } else { result = (lhs == rhs); } - this->HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } if (argP1 != newArgs.end() && argP2 != newArgs.end() && @@ -603,7 +598,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, { result = (val == 0); } - this->HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } if (argP1 != newArgs.end() && argP2 != newArgs.end() && @@ -628,7 +623,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } const auto result = cmSystemTools::VersionCompare(op, def->c_str(), def2->c_str()); - this->HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } // is file A newer than file B @@ -637,9 +632,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, auto fileIsNewer = 0; cmsys::Status ftcStatus = cmSystemTools::FileTimeCompare( arg->GetValue(), argP2->GetValue(), &fileIsNewer); - this->HandleBinaryOp( - (!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), reducible, arg, - newArgs, argP1, argP2); + HandleBinaryOp((!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), + reducible, arg, newArgs, argP1, argP2); } if (argP1 != newArgs.end() && argP2 != newArgs.end() && @@ -655,7 +649,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, result = cm::contains(cmExpandedList(*def2, true), *def); } - this->HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } else if (this->Policy57Status == cmPolicies::WARN) { std::ostringstream e; e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) << "\n"; @@ -686,7 +680,7 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { const auto rhs = this->GetBooleanValueWithAutoDereference( *argP1, errorString, status); - this->HandlePredicate(!rhs, reducible, arg, newArgs, argP1, argP2); + HandlePredicate(!rhs, reducible, arg, newArgs, argP1, argP2); } } } while (reducible); @@ -713,8 +707,7 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, this->GetBooleanValueWithAutoDereference(*arg, errorString, status); rhs = this->GetBooleanValueWithAutoDereference(*argP2, errorString, status); - this->HandleBinaryOp((lhs && rhs), reducible, arg, newArgs, argP1, - argP2); + HandleBinaryOp((lhs && rhs), reducible, arg, newArgs, argP1, argP2); } if (argP1 != newArgs.end() && this->IsKeyword(keyOR, *argP1) && @@ -723,8 +716,7 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, this->GetBooleanValueWithAutoDereference(*arg, errorString, status); rhs = this->GetBooleanValueWithAutoDereference(*argP2, errorString, status); - this->HandleBinaryOp((lhs || rhs), reducible, arg, newArgs, argP1, - argP2); + HandleBinaryOp((lhs || rhs), reducible, arg, newArgs, argP1, argP2); } } } while (reducible); diff --git a/Source/cmConditionEvaluator.h b/Source/cmConditionEvaluator.h index 74652a0..af0dc5c 100644 --- a/Source/cmConditionEvaluator.h +++ b/Source/cmConditionEvaluator.h @@ -51,16 +51,6 @@ private: MessageType& status, bool oneArg = false) const; - void HandlePredicate(bool value, bool& reducible, - cmArgumentList::iterator& arg, cmArgumentList& newArgs, - cmArgumentList::iterator& argP1, - cmArgumentList::iterator& argP2) const; - - void HandleBinaryOp(bool value, bool& reducible, - cmArgumentList::iterator& arg, cmArgumentList& newArgs, - cmArgumentList::iterator& argP1, - cmArgumentList::iterator& argP2); - bool HandleLevel0(cmArgumentList& newArgs, std::string& errorString, MessageType& status); -- cgit v0.12 From 95fc27cedde069c884d151644bf1e7ef105dc7da Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sun, 25 Jul 2021 03:41:26 +0300 Subject: Refactor: Rewrite parenthesis scanner to avoid `if`s Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index be7ae88..ef05560 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -362,18 +362,11 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, for (auto arg = newArgs.begin(); arg != newArgs.end(); ++arg) { if (this->IsKeyword(keyParenL, *arg)) { // search for the closing paren for this opening one - cmArgumentList::iterator argClose; - argClose = arg; - argClose++; - auto depth = 1u; - while (argClose != newArgs.end() && depth) { - if (this->IsKeyword(keyParenL, *argClose)) { - depth++; - } - if (this->IsKeyword(keyParenR, *argClose)) { - depth--; - } - argClose++; + auto depth = 1; + auto argClose = std::next(arg); + for (; argClose != newArgs.end() && depth; ++argClose) { + depth += int(this->IsKeyword(keyParenL, *argClose)) - + int(this->IsKeyword(keyParenR, *argClose)); } if (depth) { errorString = "mismatched parenthesis in condition"; -- cgit v0.12 From 314538703a4c70a45dc7679af6eb0c46f39ee3e4 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sun, 25 Jul 2021 03:50:52 +0300 Subject: Refactor: Deduplicate code for `AND` and `OR` handling in `if()` command Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index ef05560..3def347 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -687,29 +687,22 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, MessageType& status) { bool reducible; - bool lhs; - bool rhs; do { reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { IncrementArguments(newArgs, argP1, argP2); - if (argP1 != newArgs.end() && this->IsKeyword(keyAND, *argP1) && + if (argP1 != newArgs.end() && + (this->IsKeyword(keyAND, *argP1) || + this->IsKeyword(keyOR, *argP1)) && argP2 != newArgs.end()) { - lhs = + const auto lhs = this->GetBooleanValueWithAutoDereference(*arg, errorString, status); - rhs = this->GetBooleanValueWithAutoDereference(*argP2, errorString, - status); - HandleBinaryOp((lhs && rhs), reducible, arg, newArgs, argP1, argP2); - } - - if (argP1 != newArgs.end() && this->IsKeyword(keyOR, *argP1) && - argP2 != newArgs.end()) { - lhs = - this->GetBooleanValueWithAutoDereference(*arg, errorString, status); - rhs = this->GetBooleanValueWithAutoDereference(*argP2, errorString, - status); - HandleBinaryOp((lhs || rhs), reducible, arg, newArgs, argP1, argP2); + const auto rhs = this->GetBooleanValueWithAutoDereference( + *argP2, errorString, status); + HandleBinaryOp(this->IsKeyword(keyAND, *argP1) ? (lhs && rhs) + : (lhs || rhs), + reducible, arg, newArgs, argP1, argP2); } } } while (reducible); -- cgit v0.12 From 7bec39dc10e821571785e503a1c14f09db8004a9 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sun, 25 Jul 2021 03:54:44 +0300 Subject: Style: Add empty lines to increase readability Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 3def347..9183a44 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -403,7 +403,9 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { + IncrementArguments(newArgs, argP1, argP2); + // does a file exist if (this->IsKeyword(keyEXISTS, *arg) && argP1 != newArgs.end()) { HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), @@ -498,10 +500,14 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { + IncrementArguments(newArgs, argP1, argP2); + if (argP1 != newArgs.end() && argP2 != newArgs.end() && this->IsKeyword(keyMATCHES, *argP1)) { + def = this->GetDefinitionIfUnquoted(*arg); + if (!def) { def = &arg->GetValue(); } else if (cmHasLiteralPrefix(arg->GetValue(), "CMAKE_MATCH_")) { @@ -510,6 +516,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, def_buf = *def; def = &def_buf; } + const auto& rex = argP2->GetValue(); this->Makefile.ClearMatches(); cmsys::RegularExpression regEntry; @@ -520,12 +527,14 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, status = MessageType::FATAL_ERROR; return false; } + if (regEntry.find(*def)) { this->Makefile.StoreMatches(regEntry); *arg = cmExpandedCommandArgument("1", true); } else { *arg = cmExpandedCommandArgument("0", true); } + newArgs.erase(argP2); newArgs.erase(argP1); argP1 = arg; @@ -547,8 +556,10 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keyGREATER, *argP1) || this->IsKeyword(keyGREATER_EQUAL, *argP1) || this->IsKeyword(keyEQUAL, *argP1))) { + def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); + double lhs; double rhs; bool result; @@ -575,9 +586,11 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keySTRGREATER, *argP1) || this->IsKeyword(keySTRGREATER_EQUAL, *argP1) || this->IsKeyword(keySTREQUAL, *argP1))) { + def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); const int val = (*def).compare(*def2); + bool result; if (argP1->GetValue() == keySTRLESS) { result = (val < 0); @@ -600,8 +613,10 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keyVERSION_GREATER, *argP1) || this->IsKeyword(keyVERSION_GREATER_EQUAL, *argP1) || this->IsKeyword(keyVERSION_EQUAL, *argP1))) { + def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); + cmSystemTools::CompareOp op; if (argP1->GetValue() == keyVERSION_LESS) { op = cmSystemTools::OP_LESS; @@ -631,6 +646,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, if (argP1 != newArgs.end() && argP2 != newArgs.end() && this->IsKeyword(keyIN_LIST, *argP1)) { + if (this->Policy57Status != cmPolicies::OLD && this->Policy57Status != cmPolicies::WARN) { auto result = false; @@ -669,7 +685,9 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { + IncrementArguments(newArgs, argP1, argP2); + if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { const auto rhs = this->GetBooleanValueWithAutoDereference( *argP1, errorString, status); @@ -691,7 +709,9 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, reducible = false; for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { + IncrementArguments(newArgs, argP1, argP2); + if (argP1 != newArgs.end() && (this->IsKeyword(keyAND, *argP1) || this->IsKeyword(keyOR, *argP1)) && -- cgit v0.12 From bf2fe903728a56755d045c768fc6cc0d46845cc5 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 21:45:03 +0300 Subject: Refactor: Speedup predicates and binary operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before predicates and binary ops reducers use series of `if ()` blocks to match the keywords. However, if matched the currect `arg` get replaced with evaluation result, so further `if ()` is just wasting time/resources. This patch introduce a chain of `if` → `else if` → ..., so after first match the loop restarts w/ the next argument. --- Source/cmConditionEvaluator.cxx | 109 ++++++++++++++++++++++------------------ Source/cmConditionEvaluator.h | 2 +- 2 files changed, 61 insertions(+), 50 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 9183a44..0593786 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -221,7 +221,7 @@ cmProp cmConditionEvaluator::GetVariableOrString( } //========================================================================= -bool cmConditionEvaluator::IsKeyword(cm::string_view keyword, +bool cmConditionEvaluator::IsKeyword(cm::static_string_view keyword, cmExpandedCommandArgument& argument) const { if ((this->Policy54Status != cmPolicies::WARN && @@ -406,64 +406,67 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, IncrementArguments(newArgs, argP1, argP2); + // NOTE This is the only predicate that dones't need the next argument... + // Check it first! + // does a test exist (CMP0064 == WARN case) + if (this->Policy64Status == cmPolicies::WARN && + this->IsKeyword(keyTEST, *arg)) { + std::ostringstream e; + e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0064) << "\n"; + e << "TEST will be interpreted as an operator " + "when the policy is set to NEW. " + "Since the policy is not set the OLD behavior will be used."; + + this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); + } + + // NOTE Fail fast: All the predicates below require the next arg to be + // valid + if (argP1 == newArgs.end()) { + continue; + } + // does a file exist - if (this->IsKeyword(keyEXISTS, *arg) && argP1 != newArgs.end()) { + if (this->IsKeyword(keyEXISTS, *arg)) { HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), reducible, arg, newArgs, argP1, argP2); } // does a directory with this name exist - if (this->IsKeyword(keyIS_DIRECTORY, *arg) && argP1 != newArgs.end()) { + else if (this->IsKeyword(keyIS_DIRECTORY, *arg)) { HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), reducible, arg, newArgs, argP1, argP2); } // does a symlink with this name exist - if (this->IsKeyword(keyIS_SYMLINK, *arg) && argP1 != newArgs.end()) { + else if (this->IsKeyword(keyIS_SYMLINK, *arg)) { HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), reducible, arg, newArgs, argP1, argP2); } // is the given path an absolute path ? - if (this->IsKeyword(keyIS_ABSOLUTE, *arg) && argP1 != newArgs.end()) { + else if (this->IsKeyword(keyIS_ABSOLUTE, *arg)) { HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), reducible, arg, newArgs, argP1, argP2); } // does a command exist - if (this->IsKeyword(keyCOMMAND, *arg) && argP1 != newArgs.end()) { + else if (this->IsKeyword(keyCOMMAND, *arg)) { HandlePredicate( this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, reducible, arg, newArgs, argP1, argP2); } // does a policy exist - if (this->IsKeyword(keyPOLICY, *arg) && argP1 != newArgs.end()) { + else if (this->IsKeyword(keyPOLICY, *arg)) { cmPolicies::PolicyID pid; HandlePredicate( cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), reducible, arg, newArgs, argP1, argP2); } // does a target exist - if (this->IsKeyword(keyTARGET, *arg) && argP1 != newArgs.end()) { + else if (this->IsKeyword(keyTARGET, *arg)) { HandlePredicate(this->Makefile.FindTargetToUse(argP1->GetValue()) != nullptr, reducible, arg, newArgs, argP1, argP2); } - // does a test exist - if (this->Policy64Status != cmPolicies::OLD && - this->Policy64Status != cmPolicies::WARN) { - if (this->IsKeyword(keyTEST, *arg) && argP1 != newArgs.end()) { - HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1, argP2); - } - } else if (this->Policy64Status == cmPolicies::WARN && - this->IsKeyword(keyTEST, *arg)) { - std::ostringstream e; - e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0064) << "\n"; - e << "TEST will be interpreted as an operator " - "when the policy is set to NEW. " - "Since the policy is not set the OLD behavior will be used."; - - this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); - } // is a variable defined - if (this->IsKeyword(keyDEFINED, *arg) && argP1 != newArgs.end()) { + else if (this->IsKeyword(keyDEFINED, *arg)) { const auto argP1len = argP1->GetValue().size(); auto bdef = false; if (argP1len > 4 && cmHasLiteralPrefix(argP1->GetValue(), "ENV{") && @@ -481,6 +484,14 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } HandlePredicate(bdef, reducible, arg, newArgs, argP1, argP2); } + // does a test exist + else if (this->Policy64Status != cmPolicies::OLD && + this->Policy64Status != cmPolicies::WARN) { + if (this->IsKeyword(keyTEST, *arg)) { + HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, + reducible, arg, newArgs, argP1, argP2); + } + } } } while (reducible); return true; @@ -542,7 +553,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, reducible = true; } - if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { + else if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { *arg = cmExpandedCommandArgument("0", true); newArgs.erase(argP1); argP1 = arg; @@ -550,12 +561,16 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, reducible = true; } - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - (this->IsKeyword(keyLESS, *argP1) || - this->IsKeyword(keyLESS_EQUAL, *argP1) || - this->IsKeyword(keyGREATER, *argP1) || - this->IsKeyword(keyGREATER_EQUAL, *argP1) || - this->IsKeyword(keyEQUAL, *argP1))) { + // NOTE Fail fast: All the binary ops below require 2 arguments. + else if (argP1 == newArgs.end() || argP2 == newArgs.end()) { + continue; + } + + else if (this->IsKeyword(keyLESS, *argP1) || + this->IsKeyword(keyLESS_EQUAL, *argP1) || + this->IsKeyword(keyGREATER, *argP1) || + this->IsKeyword(keyGREATER_EQUAL, *argP1) || + this->IsKeyword(keyEQUAL, *argP1)) { def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); @@ -580,12 +595,11 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - (this->IsKeyword(keySTRLESS, *argP1) || - this->IsKeyword(keySTRLESS_EQUAL, *argP1) || - this->IsKeyword(keySTRGREATER, *argP1) || - this->IsKeyword(keySTRGREATER_EQUAL, *argP1) || - this->IsKeyword(keySTREQUAL, *argP1))) { + else if (this->IsKeyword(keySTRLESS, *argP1) || + this->IsKeyword(keySTRLESS_EQUAL, *argP1) || + this->IsKeyword(keySTRGREATER, *argP1) || + this->IsKeyword(keySTRGREATER_EQUAL, *argP1) || + this->IsKeyword(keySTREQUAL, *argP1)) { def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); @@ -607,12 +621,11 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); } - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - (this->IsKeyword(keyVERSION_LESS, *argP1) || - this->IsKeyword(keyVERSION_LESS_EQUAL, *argP1) || - this->IsKeyword(keyVERSION_GREATER, *argP1) || - this->IsKeyword(keyVERSION_GREATER_EQUAL, *argP1) || - this->IsKeyword(keyVERSION_EQUAL, *argP1))) { + else if (this->IsKeyword(keyVERSION_LESS, *argP1) || + this->IsKeyword(keyVERSION_LESS_EQUAL, *argP1) || + this->IsKeyword(keyVERSION_GREATER, *argP1) || + this->IsKeyword(keyVERSION_GREATER_EQUAL, *argP1) || + this->IsKeyword(keyVERSION_EQUAL, *argP1)) { def = this->GetVariableOrString(*arg); def2 = this->GetVariableOrString(*argP2); @@ -635,8 +648,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } // is file A newer than file B - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - this->IsKeyword(keyIS_NEWER_THAN, *argP1)) { + else if (this->IsKeyword(keyIS_NEWER_THAN, *argP1)) { auto fileIsNewer = 0; cmsys::Status ftcStatus = cmSystemTools::FileTimeCompare( arg->GetValue(), argP2->GetValue(), &fileIsNewer); @@ -644,8 +656,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, reducible, arg, newArgs, argP1, argP2); } - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - this->IsKeyword(keyIN_LIST, *argP1)) { + else if (this->IsKeyword(keyIN_LIST, *argP1)) { if (this->Policy57Status != cmPolicies::OLD && this->Policy57Status != cmPolicies::WARN) { diff --git a/Source/cmConditionEvaluator.h b/Source/cmConditionEvaluator.h index af0dc5c..a7e3ce8 100644 --- a/Source/cmConditionEvaluator.h +++ b/Source/cmConditionEvaluator.h @@ -38,7 +38,7 @@ private: cmProp GetVariableOrString(const cmExpandedCommandArgument& argument) const; - bool IsKeyword(cm::string_view keyword, + bool IsKeyword(cm::static_string_view keyword, cmExpandedCommandArgument& argument) const; bool GetBooleanValue(cmExpandedCommandArgument& arg) const; -- cgit v0.12 From 9946ff68481a1d02a75b646f3b275f98ca616d64 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Sun, 25 Jul 2021 19:07:36 +0300 Subject: Refactor: Initialize args vector from iterators instead of copy Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 0593786..871acd1 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -374,12 +374,10 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, return false; } // store the reduced args in this vector - std::vector newArgs2; - - // copy to the list structure auto argP1 = std::next(arg); - cm::append(newArgs2, argP1, argClose); + std::vector newArgs2{ argP1, argClose }; newArgs2.pop_back(); + // now recursively invoke IsTrue to handle the values inside the // parenthetical expression const auto value = this->IsTrue(newArgs2, errorString, status); -- cgit v0.12 From 8bc4a740d648e59d6712bbaea0cae8e03fc97a25 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 21:52:24 +0300 Subject: Refactor: When handle predicates, there is no need to check 2nd arg Introduce an overload for `IncrementArguments()` w/ one iterator and use it in the handler level 1. --- Source/cmConditionEvaluator.cxx | 64 ++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 871acd1..c4d945b 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -59,29 +59,34 @@ auto const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"_s; std::array const ZERO_ONE_XLAT = { "0", "1" }; -inline void IncrementArguments( - cmConditionEvaluator::cmArgumentList& newArgs, - cmConditionEvaluator::cmArgumentList::iterator& argP1, - cmConditionEvaluator::cmArgumentList::iterator& argP2) +void IncrementArguments(cmConditionEvaluator::cmArgumentList& newArgs, + cmConditionEvaluator::cmArgumentList::iterator& argP1) { - if (argP1 != newArgs.end()) { - argP2 = ++argP1; - using difference_type = - cmConditionEvaluator::cmArgumentList::difference_type; - std::advance(argP2, difference_type(argP1 != newArgs.end())); - } + using difference_type = + cmConditionEvaluator::cmArgumentList::difference_type; + std::advance(argP1, difference_type(argP1 != newArgs.end())); +} + +void IncrementArguments(cmConditionEvaluator::cmArgumentList& newArgs, + cmConditionEvaluator::cmArgumentList::iterator& argP1, + cmConditionEvaluator::cmArgumentList::iterator& argP2) +{ + IncrementArguments(newArgs, argP1); + argP2 = argP1; + using difference_type = + cmConditionEvaluator::cmArgumentList::difference_type; + std::advance(argP2, difference_type(argP1 != newArgs.end())); } void HandlePredicate(const bool value, bool& reducible, cmConditionEvaluator::cmArgumentList::iterator& arg, cmConditionEvaluator::cmArgumentList& newArgs, - cmConditionEvaluator::cmArgumentList::iterator& argP1, - cmConditionEvaluator::cmArgumentList::iterator& argP2) + cmConditionEvaluator::cmArgumentList::iterator& argP1) { *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); newArgs.erase(argP1); argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1); reducible = true; } @@ -98,7 +103,6 @@ void HandleBinaryOp(const bool value, bool& reducible, IncrementArguments(newArgs, argP1, argP2); reducible = true; } - } // anonymous namespace cmConditionEvaluator::cmConditionEvaluator(cmMakefile& makefile, @@ -399,10 +403,10 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, bool reducible; do { reducible = false; - for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; - arg != newArgs.end(); argP1 = ++arg) { + for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); + argP1 = ++arg) { - IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1); // NOTE This is the only predicate that dones't need the next argument... // Check it first! @@ -427,41 +431,41 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, // does a file exist if (this->IsKeyword(keyEXISTS, *arg)) { HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), - reducible, arg, newArgs, argP1, argP2); + reducible, arg, newArgs, argP1); } // does a directory with this name exist else if (this->IsKeyword(keyIS_DIRECTORY, *arg)) { HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), - reducible, arg, newArgs, argP1, argP2); + reducible, arg, newArgs, argP1); } // does a symlink with this name exist else if (this->IsKeyword(keyIS_SYMLINK, *arg)) { HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), - reducible, arg, newArgs, argP1, argP2); + reducible, arg, newArgs, argP1); } // is the given path an absolute path ? else if (this->IsKeyword(keyIS_ABSOLUTE, *arg)) { HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), - reducible, arg, newArgs, argP1, argP2); + reducible, arg, newArgs, argP1); } // does a command exist else if (this->IsKeyword(keyCOMMAND, *arg)) { HandlePredicate( this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1, argP2); + reducible, arg, newArgs, argP1); } // does a policy exist else if (this->IsKeyword(keyPOLICY, *arg)) { cmPolicies::PolicyID pid; HandlePredicate( cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), reducible, - arg, newArgs, argP1, argP2); + arg, newArgs, argP1); } // does a target exist else if (this->IsKeyword(keyTARGET, *arg)) { HandlePredicate(this->Makefile.FindTargetToUse(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1, argP2); + reducible, arg, newArgs, argP1); } // is a variable defined else if (this->IsKeyword(keyDEFINED, *arg)) { @@ -480,14 +484,14 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } else { bdef = this->Makefile.IsDefinitionSet(argP1->GetValue()); } - HandlePredicate(bdef, reducible, arg, newArgs, argP1, argP2); + HandlePredicate(bdef, reducible, arg, newArgs, argP1); } // does a test exist else if (this->Policy64Status != cmPolicies::OLD && this->Policy64Status != cmPolicies::WARN) { if (this->IsKeyword(keyTEST, *arg)) { HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1, argP2); + reducible, arg, newArgs, argP1); } } } @@ -692,15 +696,15 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, bool reducible; do { reducible = false; - for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; - arg != newArgs.end(); argP1 = ++arg) { + for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); + argP1 = ++arg) { - IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1); if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { const auto rhs = this->GetBooleanValueWithAutoDereference( *argP1, errorString, status); - HandlePredicate(!rhs, reducible, arg, newArgs, argP1, argP2); + HandlePredicate(!rhs, reducible, arg, newArgs, argP1); } } } while (reducible); -- cgit v0.12 From 18bd6c98ea70adf3067539266fdb782e910fb3cc Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 21:56:52 +0300 Subject: Refactor: Generalize policy checking in `HandleLevel1` Also, move OLD policy checking out of the loop to evaluate it once. --- Source/cmConditionEvaluator.cxx | 43 ++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index c4d945b..99927d7 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -400,6 +400,9 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, MessageType&) { + const auto policy64IsOld = this->Policy64Status == cmPolicies::OLD || + this->Policy64Status == cmPolicies::WARN; + bool reducible; do { reducible = false; @@ -407,20 +410,24 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, argP1 = ++arg) { IncrementArguments(newArgs, argP1); + auto policyCheck = [&, this](const cmPolicies::PolicyID id, + const cmPolicies::PolicyStatus status, + const cm::static_string_view kw) { + if (status == cmPolicies::WARN && this->IsKeyword(kw, *arg)) { + std::ostringstream e; + e << cmPolicies::GetPolicyWarning(id) << "\n" + << kw + << " will be interpreted as an operator " + "when the policy is set to NEW. " + "Since the policy is not set the OLD behavior will be used."; - // NOTE This is the only predicate that dones't need the next argument... - // Check it first! - // does a test exist (CMP0064 == WARN case) - if (this->Policy64Status == cmPolicies::WARN && - this->IsKeyword(keyTEST, *arg)) { - std::ostringstream e; - e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0064) << "\n"; - e << "TEST will be interpreted as an operator " - "when the policy is set to NEW. " - "Since the policy is not set the OLD behavior will be used."; - - this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); - } + this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); + } + }; + + // NOTE Checking policies for warnings are not require an access to the + // next arg. Check them first! + policyCheck(cmPolicies::CMP0064, this->Policy64Status, keyTEST); // NOTE Fail fast: All the predicates below require the next arg to be // valid @@ -487,12 +494,12 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, HandlePredicate(bdef, reducible, arg, newArgs, argP1); } // does a test exist - else if (this->Policy64Status != cmPolicies::OLD && - this->Policy64Status != cmPolicies::WARN) { - if (this->IsKeyword(keyTEST, *arg)) { - HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1); + else if (this->IsKeyword(keyTEST, *arg)) { + if (policy64IsOld) { + continue; } + HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, + reducible, arg, newArgs, argP1); } } } while (reducible); -- cgit v0.12 From b0d65963992d85349487a3fdf51cfced6910384d Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 01:54:24 +0300 Subject: Refactor: Make `cmConditionEvaluator::IsTrue` a bit more compact Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 99927d7..7ea0daf 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -149,25 +149,19 @@ bool cmConditionEvaluator::IsTrue( // now loop through the arguments and see if we can reduce any of them // we do this multiple times. Once for each level of precedence // parens - if (!this->HandleLevel0(newArgs, errorString, status)) { - return false; - } - // predicates - if (!this->HandleLevel1(newArgs, errorString, status)) { - return false; - } - // binary ops - if (!this->HandleLevel2(newArgs, errorString, status)) { - return false; - } - - // NOT - if (!this->HandleLevel3(newArgs, errorString, status)) { - return false; - } - // AND OR - if (!this->HandleLevel4(newArgs, errorString, status)) { - return false; + using handlerFn_t = bool (cmConditionEvaluator::*)( + cmArgumentList&, std::string&, MessageType&); + const std::array handlers = { { + &cmConditionEvaluator::HandleLevel0, // parenthesis + &cmConditionEvaluator::HandleLevel1, // predicates + &cmConditionEvaluator::HandleLevel2, // binary ops + &cmConditionEvaluator::HandleLevel3, // NOT + &cmConditionEvaluator::HandleLevel4 // AND OR + } }; + for (auto fn : handlers) { + if (!(this->*fn)(newArgs, errorString, status)) { + return false; + } } // now at the end there should only be one argument left -- cgit v0.12 From 78fcbb20cdd6f33423e8b503a4b55de4abfa52f6 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 02:51:43 +0300 Subject: Refactor: Remove `reducible` flag from `handleLevelN()` functions The indicator that smth has been done is the `newArgs` size get differ after an iteration. Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 90 ++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 7ea0daf..c19338b 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -78,7 +78,7 @@ void IncrementArguments(cmConditionEvaluator::cmArgumentList& newArgs, std::advance(argP2, difference_type(argP1 != newArgs.end())); } -void HandlePredicate(const bool value, bool& reducible, +void HandlePredicate(const bool value, cmConditionEvaluator::cmArgumentList::iterator& arg, cmConditionEvaluator::cmArgumentList& newArgs, cmConditionEvaluator::cmArgumentList::iterator& argP1) @@ -87,10 +87,9 @@ void HandlePredicate(const bool value, bool& reducible, newArgs.erase(argP1); argP1 = arg; IncrementArguments(newArgs, argP1); - reducible = true; } -void HandleBinaryOp(const bool value, bool& reducible, +void HandleBinaryOp(const bool value, cmConditionEvaluator::cmArgumentList::iterator& arg, cmConditionEvaluator::cmArgumentList& newArgs, cmConditionEvaluator::cmArgumentList::iterator& argP1, @@ -101,7 +100,6 @@ void HandleBinaryOp(const bool value, bool& reducible, newArgs.erase(argP1); argP1 = arg; IncrementArguments(newArgs, argP1, argP2); - reducible = true; } } // anonymous namespace @@ -354,9 +352,10 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - bool reducible; + std::size_t beginSize; do { - reducible = false; + beginSize = newArgs.size(); + for (auto arg = newArgs.begin(); arg != newArgs.end(); ++arg) { if (this->IsKeyword(keyParenL, *arg)) { // search for the closing paren for this opening one @@ -385,7 +384,7 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, newArgs.erase(argP1, argClose); } } - } while (reducible); + } while (beginSize != newArgs.size()); return true; } @@ -397,9 +396,10 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, const auto policy64IsOld = this->Policy64Status == cmPolicies::OLD || this->Policy64Status == cmPolicies::WARN; - bool reducible; + std::size_t beginSize; do { - reducible = false; + beginSize = newArgs.size(); + for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); argP1 = ++arg) { @@ -431,42 +431,42 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, // does a file exist if (this->IsKeyword(keyEXISTS, *arg)) { - HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), - reducible, arg, newArgs, argP1); + HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), arg, + newArgs, argP1); } // does a directory with this name exist else if (this->IsKeyword(keyIS_DIRECTORY, *arg)) { - HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), - reducible, arg, newArgs, argP1); + HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), arg, + newArgs, argP1); } // does a symlink with this name exist else if (this->IsKeyword(keyIS_SYMLINK, *arg)) { - HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), - reducible, arg, newArgs, argP1); + HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), arg, + newArgs, argP1); } // is the given path an absolute path ? else if (this->IsKeyword(keyIS_ABSOLUTE, *arg)) { - HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), - reducible, arg, newArgs, argP1); + HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), arg, + newArgs, argP1); } // does a command exist else if (this->IsKeyword(keyCOMMAND, *arg)) { HandlePredicate( this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1); + arg, newArgs, argP1); } // does a policy exist else if (this->IsKeyword(keyPOLICY, *arg)) { cmPolicies::PolicyID pid; HandlePredicate( - cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), reducible, - arg, newArgs, argP1); + cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), arg, + newArgs, argP1); } // does a target exist else if (this->IsKeyword(keyTARGET, *arg)) { HandlePredicate(this->Makefile.FindTargetToUse(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1); + arg, newArgs, argP1); } // is a variable defined else if (this->IsKeyword(keyDEFINED, *arg)) { @@ -485,7 +485,7 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } else { bdef = this->Makefile.IsDefinitionSet(argP1->GetValue()); } - HandlePredicate(bdef, reducible, arg, newArgs, argP1); + HandlePredicate(bdef, arg, newArgs, argP1); } // does a test exist else if (this->IsKeyword(keyTEST, *arg)) { @@ -493,10 +493,10 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, continue; } HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, - reducible, arg, newArgs, argP1); + arg, newArgs, argP1); } } - } while (reducible); + } while (beginSize != newArgs.size()); return true; } @@ -506,12 +506,13 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - bool reducible; std::string def_buf; cmProp def; cmProp def2; + std::size_t beginSize; do { - reducible = false; + beginSize = newArgs.size(); + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { @@ -553,7 +554,6 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, newArgs.erase(argP1); argP1 = arg; IncrementArguments(newArgs, argP1, argP2); - reducible = true; } else if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { @@ -561,7 +561,6 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, newArgs.erase(argP1); argP1 = arg; IncrementArguments(newArgs, argP1, argP2); - reducible = true; } // NOTE Fail fast: All the binary ops below require 2 arguments. @@ -595,7 +594,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } else { result = (lhs == rhs); } - HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, arg, newArgs, argP1, argP2); } else if (this->IsKeyword(keySTRLESS, *argP1) || @@ -621,7 +620,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, { result = (val == 0); } - HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, arg, newArgs, argP1, argP2); } else if (this->IsKeyword(keyVERSION_LESS, *argP1) || @@ -647,7 +646,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } const auto result = cmSystemTools::VersionCompare(op, def->c_str(), def2->c_str()); - HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, arg, newArgs, argP1, argP2); } // is file A newer than file B @@ -656,7 +655,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, cmsys::Status ftcStatus = cmSystemTools::FileTimeCompare( arg->GetValue(), argP2->GetValue(), &fileIsNewer); HandleBinaryOp((!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), - reducible, arg, newArgs, argP1, argP2); + arg, newArgs, argP1, argP2); } else if (this->IsKeyword(keyIN_LIST, *argP1)) { @@ -672,7 +671,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, result = cm::contains(cmExpandedList(*def2, true), *def); } - HandleBinaryOp(result, reducible, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, arg, newArgs, argP1, argP2); } else if (this->Policy57Status == cmPolicies::WARN) { std::ostringstream e; e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) << "\n"; @@ -684,7 +683,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } } } - } while (reducible); + } while (beginSize != newArgs.size()); return true; } @@ -694,9 +693,10 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - bool reducible; + std::size_t beginSize; do { - reducible = false; + beginSize = newArgs.size(); + for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); argP1 = ++arg) { @@ -705,10 +705,10 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { const auto rhs = this->GetBooleanValueWithAutoDereference( *argP1, errorString, status); - HandlePredicate(!rhs, reducible, arg, newArgs, argP1); + HandlePredicate(!rhs, arg, newArgs, argP1); } } - } while (reducible); + } while (beginSize != newArgs.size()); return true; } @@ -718,27 +718,27 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - bool reducible; + std::size_t beginSize; do { - reducible = false; + beginSize = newArgs.size(); + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { IncrementArguments(newArgs, argP1, argP2); - if (argP1 != newArgs.end() && + if (argP1 != newArgs.end() && argP2 != newArgs.end() && (this->IsKeyword(keyAND, *argP1) || - this->IsKeyword(keyOR, *argP1)) && - argP2 != newArgs.end()) { + this->IsKeyword(keyOR, *argP1))) { const auto lhs = this->GetBooleanValueWithAutoDereference(*arg, errorString, status); const auto rhs = this->GetBooleanValueWithAutoDereference( *argP2, errorString, status); HandleBinaryOp(this->IsKeyword(keyAND, *argP1) ? (lhs && rhs) : (lhs || rhs), - reducible, arg, newArgs, argP1, argP2); + arg, newArgs, argP1, argP2); } } - } while (reducible); + } while (beginSize != newArgs.size()); return true; } -- cgit v0.12 From 4de2a4a46dcddd3e86c6da8be8a3fe2008b093cf Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 04:02:04 +0300 Subject: Refactor: Opt-out do+while loops and reduce nesting level in handlers Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 596 +++++++++++++++++++--------------------- 1 file changed, 287 insertions(+), 309 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index c19338b..5f66dbf 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -157,8 +157,12 @@ bool cmConditionEvaluator::IsTrue( &cmConditionEvaluator::HandleLevel4 // AND OR } }; for (auto fn : handlers) { - if (!(this->*fn)(newArgs, errorString, status)) { - return false; + // Call the reducer 'till there is anything to reduce... + // (i.e., if after an iteration the size becomes smaller) + for (auto beginSize = newArgs.size(); + (this->*fn)(newArgs, errorString, status) && + newArgs.size() < beginSize; + beginSize = newArgs.size()) { } } @@ -352,39 +356,34 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - std::size_t beginSize; - do { - beginSize = newArgs.size(); - - for (auto arg = newArgs.begin(); arg != newArgs.end(); ++arg) { - if (this->IsKeyword(keyParenL, *arg)) { - // search for the closing paren for this opening one - auto depth = 1; - auto argClose = std::next(arg); - for (; argClose != newArgs.end() && depth; ++argClose) { - depth += int(this->IsKeyword(keyParenL, *argClose)) - - int(this->IsKeyword(keyParenR, *argClose)); - } - if (depth) { - errorString = "mismatched parenthesis in condition"; - status = MessageType::FATAL_ERROR; - return false; - } - // store the reduced args in this vector - auto argP1 = std::next(arg); - std::vector newArgs2{ argP1, argClose }; - newArgs2.pop_back(); - - // now recursively invoke IsTrue to handle the values inside the - // parenthetical expression - const auto value = this->IsTrue(newArgs2, errorString, status); - *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); - argP1 = std::next(arg); - // remove the now evaluated parenthetical expression - newArgs.erase(argP1, argClose); + for (auto arg = newArgs.begin(); arg != newArgs.end(); ++arg) { + if (this->IsKeyword(keyParenL, *arg)) { + // search for the closing paren for this opening one + auto depth = 1; + auto argClose = std::next(arg); + for (; argClose != newArgs.end() && depth; ++argClose) { + depth += int(this->IsKeyword(keyParenL, *argClose)) - + int(this->IsKeyword(keyParenR, *argClose)); + } + if (depth) { + errorString = "mismatched parenthesis in condition"; + status = MessageType::FATAL_ERROR; + return false; } + // store the reduced args in this vector + auto argP1 = std::next(arg); + std::vector newArgs2{ argP1, argClose }; + newArgs2.pop_back(); + + // now recursively invoke IsTrue to handle the values inside the + // parenthetical expression + const auto value = this->IsTrue(newArgs2, errorString, status); + *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); + argP1 = std::next(arg); + // remove the now evaluated parenthetical expression + newArgs.erase(argP1, argClose); } - } while (beginSize != newArgs.size()); + } return true; } @@ -396,107 +395,101 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, const auto policy64IsOld = this->Policy64Status == cmPolicies::OLD || this->Policy64Status == cmPolicies::WARN; - std::size_t beginSize; - do { - beginSize = newArgs.size(); - - for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); - argP1 = ++arg) { - - IncrementArguments(newArgs, argP1); - auto policyCheck = [&, this](const cmPolicies::PolicyID id, - const cmPolicies::PolicyStatus status, - const cm::static_string_view kw) { - if (status == cmPolicies::WARN && this->IsKeyword(kw, *arg)) { - std::ostringstream e; - e << cmPolicies::GetPolicyWarning(id) << "\n" - << kw - << " will be interpreted as an operator " - "when the policy is set to NEW. " - "Since the policy is not set the OLD behavior will be used."; - - this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); - } - }; + for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); + argP1 = ++arg) { - // NOTE Checking policies for warnings are not require an access to the - // next arg. Check them first! - policyCheck(cmPolicies::CMP0064, this->Policy64Status, keyTEST); + IncrementArguments(newArgs, argP1); - // NOTE Fail fast: All the predicates below require the next arg to be - // valid - if (argP1 == newArgs.end()) { - continue; - } + auto policyCheck = [&, this](const cmPolicies::PolicyID id, + const cmPolicies::PolicyStatus status, + const cm::static_string_view kw) { + if (status == cmPolicies::WARN && this->IsKeyword(kw, *arg)) { + std::ostringstream e; + e << cmPolicies::GetPolicyWarning(id) << "\n" + << kw + << " will be interpreted as an operator " + "when the policy is set to NEW. " + "Since the policy is not set the OLD behavior will be used."; - // does a file exist - if (this->IsKeyword(keyEXISTS, *arg)) { - HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), arg, - newArgs, argP1); - } - // does a directory with this name exist - else if (this->IsKeyword(keyIS_DIRECTORY, *arg)) { - HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), arg, - newArgs, argP1); + this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); } - // does a symlink with this name exist - else if (this->IsKeyword(keyIS_SYMLINK, *arg)) { - HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), arg, - newArgs, argP1); - } - // is the given path an absolute path ? - else if (this->IsKeyword(keyIS_ABSOLUTE, *arg)) { - HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), arg, - newArgs, argP1); - } - // does a command exist - else if (this->IsKeyword(keyCOMMAND, *arg)) { - HandlePredicate( - this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, - arg, newArgs, argP1); - } - // does a policy exist - else if (this->IsKeyword(keyPOLICY, *arg)) { - cmPolicies::PolicyID pid; - HandlePredicate( - cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), arg, - newArgs, argP1); - } - // does a target exist - else if (this->IsKeyword(keyTARGET, *arg)) { - HandlePredicate(this->Makefile.FindTargetToUse(argP1->GetValue()) != - nullptr, - arg, newArgs, argP1); - } - // is a variable defined - else if (this->IsKeyword(keyDEFINED, *arg)) { - const auto argP1len = argP1->GetValue().size(); - auto bdef = false; - if (argP1len > 4 && cmHasLiteralPrefix(argP1->GetValue(), "ENV{") && - argP1->GetValue().operator[](argP1len - 1) == '}') { - const auto env = argP1->GetValue().substr(4, argP1len - 5); - bdef = cmSystemTools::HasEnv(env); - } else if (argP1len > 6 && - cmHasLiteralPrefix(argP1->GetValue(), "CACHE{") && - argP1->GetValue().operator[](argP1len - 1) == '}') { - const auto cache = argP1->GetValue().substr(6, argP1len - 7); - bdef = - this->Makefile.GetState()->GetCacheEntryValue(cache) != nullptr; - } else { - bdef = this->Makefile.IsDefinitionSet(argP1->GetValue()); - } - HandlePredicate(bdef, arg, newArgs, argP1); + }; + + // NOTE Checking policies for warnings are not require an access to the + // next arg. Check them first! + policyCheck(cmPolicies::CMP0064, this->Policy64Status, keyTEST); + + // NOTE Fail fast: All the predicates below require the next arg to be + // valid + if (argP1 == newArgs.end()) { + continue; + } + + // does a file exist + if (this->IsKeyword(keyEXISTS, *arg)) { + HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), arg, + newArgs, argP1); + } + // does a directory with this name exist + else if (this->IsKeyword(keyIS_DIRECTORY, *arg)) { + HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), arg, + newArgs, argP1); + } + // does a symlink with this name exist + else if (this->IsKeyword(keyIS_SYMLINK, *arg)) { + HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), arg, + newArgs, argP1); + } + // is the given path an absolute path ? + else if (this->IsKeyword(keyIS_ABSOLUTE, *arg)) { + HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), arg, + newArgs, argP1); + } + // does a command exist + else if (this->IsKeyword(keyCOMMAND, *arg)) { + HandlePredicate( + this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, + arg, newArgs, argP1); + } + // does a policy exist + else if (this->IsKeyword(keyPOLICY, *arg)) { + cmPolicies::PolicyID pid; + HandlePredicate(cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), + arg, newArgs, argP1); + } + // does a target exist + else if (this->IsKeyword(keyTARGET, *arg)) { + HandlePredicate(this->Makefile.FindTargetToUse(argP1->GetValue()) != + nullptr, + arg, newArgs, argP1); + } + // is a variable defined + else if (this->IsKeyword(keyDEFINED, *arg)) { + const auto argP1len = argP1->GetValue().size(); + auto bdef = false; + if (argP1len > 4 && cmHasLiteralPrefix(argP1->GetValue(), "ENV{") && + argP1->GetValue().operator[](argP1len - 1) == '}') { + const auto env = argP1->GetValue().substr(4, argP1len - 5); + bdef = cmSystemTools::HasEnv(env); + } else if (argP1len > 6 && + cmHasLiteralPrefix(argP1->GetValue(), "CACHE{") && + argP1->GetValue().operator[](argP1len - 1) == '}') { + const auto cache = argP1->GetValue().substr(6, argP1len - 7); + bdef = this->Makefile.GetState()->GetCacheEntryValue(cache) != nullptr; + } else { + bdef = this->Makefile.IsDefinitionSet(argP1->GetValue()); } - // does a test exist - else if (this->IsKeyword(keyTEST, *arg)) { - if (policy64IsOld) { - continue; - } - HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, - arg, newArgs, argP1); + HandlePredicate(bdef, arg, newArgs, argP1); + } + // does a test exist + else if (this->IsKeyword(keyTEST, *arg)) { + if (policy64IsOld) { + continue; } + HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, + arg, newArgs, argP1); } - } while (beginSize != newArgs.size()); + } return true; } @@ -509,181 +502,177 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, std::string def_buf; cmProp def; cmProp def2; - std::size_t beginSize; - do { - beginSize = newArgs.size(); - for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; - arg != newArgs.end(); argP1 = ++arg) { + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; + arg != newArgs.end(); argP1 = ++arg) { - IncrementArguments(newArgs, argP1, argP2); + IncrementArguments(newArgs, argP1, argP2); - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - this->IsKeyword(keyMATCHES, *argP1)) { + if (argP1 != newArgs.end() && argP2 != newArgs.end() && + this->IsKeyword(keyMATCHES, *argP1)) { - def = this->GetDefinitionIfUnquoted(*arg); - - if (!def) { - def = &arg->GetValue(); - } else if (cmHasLiteralPrefix(arg->GetValue(), "CMAKE_MATCH_")) { - // The string to match is owned by our match result variables. - // Move it to our own buffer before clearing them. - def_buf = *def; - def = &def_buf; - } - - const auto& rex = argP2->GetValue(); - this->Makefile.ClearMatches(); - cmsys::RegularExpression regEntry; - if (!regEntry.compile(rex)) { - std::ostringstream error; - error << "Regular expression \"" << rex << "\" cannot compile"; - errorString = error.str(); - status = MessageType::FATAL_ERROR; - return false; - } - - if (regEntry.find(*def)) { - this->Makefile.StoreMatches(regEntry); - *arg = cmExpandedCommandArgument("1", true); - } else { - *arg = cmExpandedCommandArgument("0", true); - } + def = this->GetDefinitionIfUnquoted(*arg); - newArgs.erase(argP2); - newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); + if (!def) { + def = &arg->GetValue(); + } else if (cmHasLiteralPrefix(arg->GetValue(), "CMAKE_MATCH_")) { + // The string to match is owned by our match result variables. + // Move it to our own buffer before clearing them. + def_buf = *def; + def = &def_buf; } - else if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { - *arg = cmExpandedCommandArgument("0", true); - newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); + const auto& rex = argP2->GetValue(); + this->Makefile.ClearMatches(); + cmsys::RegularExpression regEntry; + if (!regEntry.compile(rex)) { + std::ostringstream error; + error << "Regular expression \"" << rex << "\" cannot compile"; + errorString = error.str(); + status = MessageType::FATAL_ERROR; + return false; } - // NOTE Fail fast: All the binary ops below require 2 arguments. - else if (argP1 == newArgs.end() || argP2 == newArgs.end()) { - continue; + if (regEntry.find(*def)) { + this->Makefile.StoreMatches(regEntry); + *arg = cmExpandedCommandArgument("1", true); + } else { + *arg = cmExpandedCommandArgument("0", true); } - else if (this->IsKeyword(keyLESS, *argP1) || - this->IsKeyword(keyLESS_EQUAL, *argP1) || - this->IsKeyword(keyGREATER, *argP1) || - this->IsKeyword(keyGREATER_EQUAL, *argP1) || - this->IsKeyword(keyEQUAL, *argP1)) { + newArgs.erase(argP2); + newArgs.erase(argP1); + argP1 = arg; + IncrementArguments(newArgs, argP1, argP2); + } - def = this->GetVariableOrString(*arg); - def2 = this->GetVariableOrString(*argP2); - - double lhs; - double rhs; - bool result; - if (std::sscanf(def->c_str(), "%lg", &lhs) != 1 || - std::sscanf(def2->c_str(), "%lg", &rhs) != 1) { - result = false; - } else if (argP1->GetValue() == keyLESS) { - result = (lhs < rhs); - } else if (argP1->GetValue() == keyLESS_EQUAL) { - result = (lhs <= rhs); - } else if (argP1->GetValue() == keyGREATER) { - result = (lhs > rhs); - } else if (argP1->GetValue() == keyGREATER_EQUAL) { - result = (lhs >= rhs); - } else { - result = (lhs == rhs); - } - HandleBinaryOp(result, arg, newArgs, argP1, argP2); - } + else if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { + *arg = cmExpandedCommandArgument("0", true); + newArgs.erase(argP1); + argP1 = arg; + IncrementArguments(newArgs, argP1, argP2); + } - else if (this->IsKeyword(keySTRLESS, *argP1) || - this->IsKeyword(keySTRLESS_EQUAL, *argP1) || - this->IsKeyword(keySTRGREATER, *argP1) || - this->IsKeyword(keySTRGREATER_EQUAL, *argP1) || - this->IsKeyword(keySTREQUAL, *argP1)) { + // NOTE Fail fast: All the binary ops below require 2 arguments. + else if (argP1 == newArgs.end() || argP2 == newArgs.end()) { + continue; + } - def = this->GetVariableOrString(*arg); - def2 = this->GetVariableOrString(*argP2); - const int val = (*def).compare(*def2); - - bool result; - if (argP1->GetValue() == keySTRLESS) { - result = (val < 0); - } else if (argP1->GetValue() == keySTRLESS_EQUAL) { - result = (val <= 0); - } else if (argP1->GetValue() == keySTRGREATER) { - result = (val > 0); - } else if (argP1->GetValue() == keySTRGREATER_EQUAL) { - result = (val >= 0); - } else // strequal - { - result = (val == 0); - } - HandleBinaryOp(result, arg, newArgs, argP1, argP2); + else if (this->IsKeyword(keyLESS, *argP1) || + this->IsKeyword(keyLESS_EQUAL, *argP1) || + this->IsKeyword(keyGREATER, *argP1) || + this->IsKeyword(keyGREATER_EQUAL, *argP1) || + this->IsKeyword(keyEQUAL, *argP1)) { + + def = this->GetVariableOrString(*arg); + def2 = this->GetVariableOrString(*argP2); + + double lhs; + double rhs; + bool result; + if (std::sscanf(def->c_str(), "%lg", &lhs) != 1 || + std::sscanf(def2->c_str(), "%lg", &rhs) != 1) { + result = false; + } else if (argP1->GetValue() == keyLESS) { + result = (lhs < rhs); + } else if (argP1->GetValue() == keyLESS_EQUAL) { + result = (lhs <= rhs); + } else if (argP1->GetValue() == keyGREATER) { + result = (lhs > rhs); + } else if (argP1->GetValue() == keyGREATER_EQUAL) { + result = (lhs >= rhs); + } else { + result = (lhs == rhs); } + HandleBinaryOp(result, arg, newArgs, argP1, argP2); + } - else if (this->IsKeyword(keyVERSION_LESS, *argP1) || - this->IsKeyword(keyVERSION_LESS_EQUAL, *argP1) || - this->IsKeyword(keyVERSION_GREATER, *argP1) || - this->IsKeyword(keyVERSION_GREATER_EQUAL, *argP1) || - this->IsKeyword(keyVERSION_EQUAL, *argP1)) { - - def = this->GetVariableOrString(*arg); - def2 = this->GetVariableOrString(*argP2); - - cmSystemTools::CompareOp op; - if (argP1->GetValue() == keyVERSION_LESS) { - op = cmSystemTools::OP_LESS; - } else if (argP1->GetValue() == keyVERSION_LESS_EQUAL) { - op = cmSystemTools::OP_LESS_EQUAL; - } else if (argP1->GetValue() == keyVERSION_GREATER) { - op = cmSystemTools::OP_GREATER; - } else if (argP1->GetValue() == keyVERSION_GREATER_EQUAL) { - op = cmSystemTools::OP_GREATER_EQUAL; - } else { // version_equal - op = cmSystemTools::OP_EQUAL; - } - const auto result = - cmSystemTools::VersionCompare(op, def->c_str(), def2->c_str()); - HandleBinaryOp(result, arg, newArgs, argP1, argP2); + else if (this->IsKeyword(keySTRLESS, *argP1) || + this->IsKeyword(keySTRLESS_EQUAL, *argP1) || + this->IsKeyword(keySTRGREATER, *argP1) || + this->IsKeyword(keySTRGREATER_EQUAL, *argP1) || + this->IsKeyword(keySTREQUAL, *argP1)) { + + def = this->GetVariableOrString(*arg); + def2 = this->GetVariableOrString(*argP2); + const int val = (*def).compare(*def2); + + bool result; + if (argP1->GetValue() == keySTRLESS) { + result = (val < 0); + } else if (argP1->GetValue() == keySTRLESS_EQUAL) { + result = (val <= 0); + } else if (argP1->GetValue() == keySTRGREATER) { + result = (val > 0); + } else if (argP1->GetValue() == keySTRGREATER_EQUAL) { + result = (val >= 0); + } else // strequal + { + result = (val == 0); } + HandleBinaryOp(result, arg, newArgs, argP1, argP2); + } - // is file A newer than file B - else if (this->IsKeyword(keyIS_NEWER_THAN, *argP1)) { - auto fileIsNewer = 0; - cmsys::Status ftcStatus = cmSystemTools::FileTimeCompare( - arg->GetValue(), argP2->GetValue(), &fileIsNewer); - HandleBinaryOp((!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), - arg, newArgs, argP1, argP2); + else if (this->IsKeyword(keyVERSION_LESS, *argP1) || + this->IsKeyword(keyVERSION_LESS_EQUAL, *argP1) || + this->IsKeyword(keyVERSION_GREATER, *argP1) || + this->IsKeyword(keyVERSION_GREATER_EQUAL, *argP1) || + this->IsKeyword(keyVERSION_EQUAL, *argP1)) { + + def = this->GetVariableOrString(*arg); + def2 = this->GetVariableOrString(*argP2); + + cmSystemTools::CompareOp op; + if (argP1->GetValue() == keyVERSION_LESS) { + op = cmSystemTools::OP_LESS; + } else if (argP1->GetValue() == keyVERSION_LESS_EQUAL) { + op = cmSystemTools::OP_LESS_EQUAL; + } else if (argP1->GetValue() == keyVERSION_GREATER) { + op = cmSystemTools::OP_GREATER; + } else if (argP1->GetValue() == keyVERSION_GREATER_EQUAL) { + op = cmSystemTools::OP_GREATER_EQUAL; + } else { // version_equal + op = cmSystemTools::OP_EQUAL; } + const auto result = + cmSystemTools::VersionCompare(op, def->c_str(), def2->c_str()); + HandleBinaryOp(result, arg, newArgs, argP1, argP2); + } - else if (this->IsKeyword(keyIN_LIST, *argP1)) { - - if (this->Policy57Status != cmPolicies::OLD && - this->Policy57Status != cmPolicies::WARN) { - auto result = false; + // is file A newer than file B + else if (this->IsKeyword(keyIS_NEWER_THAN, *argP1)) { + auto fileIsNewer = 0; + cmsys::Status ftcStatus = cmSystemTools::FileTimeCompare( + arg->GetValue(), argP2->GetValue(), &fileIsNewer); + HandleBinaryOp((!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), arg, + newArgs, argP1, argP2); + } - def = this->GetVariableOrString(*arg); - def2 = this->Makefile.GetDefinition(argP2->GetValue()); + else if (this->IsKeyword(keyIN_LIST, *argP1)) { - if (def2) { - result = cm::contains(cmExpandedList(*def2, true), *def); - } + if (this->Policy57Status != cmPolicies::OLD && + this->Policy57Status != cmPolicies::WARN) { + auto result = false; - HandleBinaryOp(result, arg, newArgs, argP1, argP2); - } else if (this->Policy57Status == cmPolicies::WARN) { - std::ostringstream e; - e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) << "\n"; - e << "IN_LIST will be interpreted as an operator " - "when the policy is set to NEW. " - "Since the policy is not set the OLD behavior will be used."; + def = this->GetVariableOrString(*arg); + def2 = this->Makefile.GetDefinition(argP2->GetValue()); - this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); + if (def2) { + result = cm::contains(cmExpandedList(*def2, true), *def); } + + HandleBinaryOp(result, arg, newArgs, argP1, argP2); + } else if (this->Policy57Status == cmPolicies::WARN) { + std::ostringstream e; + e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) << "\n"; + e << "IN_LIST will be interpreted as an operator " + "when the policy is set to NEW. " + "Since the policy is not set the OLD behavior will be used."; + + this->Makefile.IssueMessage(MessageType::AUTHOR_WARNING, e.str()); } } - } while (beginSize != newArgs.size()); + } return true; } @@ -693,22 +682,17 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - std::size_t beginSize; - do { - beginSize = newArgs.size(); + for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); + argP1 = ++arg) { - for (auto arg = newArgs.begin(), argP1 = arg; arg != newArgs.end(); - argP1 = ++arg) { + IncrementArguments(newArgs, argP1); - IncrementArguments(newArgs, argP1); - - if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { - const auto rhs = this->GetBooleanValueWithAutoDereference( - *argP1, errorString, status); - HandlePredicate(!rhs, arg, newArgs, argP1); - } + if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { + const auto rhs = + this->GetBooleanValueWithAutoDereference(*argP1, errorString, status); + HandlePredicate(!rhs, arg, newArgs, argP1); } - } while (beginSize != newArgs.size()); + } return true; } @@ -718,27 +702,21 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - std::size_t beginSize; - do { - beginSize = newArgs.size(); - - for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; - arg != newArgs.end(); argP1 = ++arg) { - - IncrementArguments(newArgs, argP1, argP2); - - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - (this->IsKeyword(keyAND, *argP1) || - this->IsKeyword(keyOR, *argP1))) { - const auto lhs = - this->GetBooleanValueWithAutoDereference(*arg, errorString, status); - const auto rhs = this->GetBooleanValueWithAutoDereference( - *argP2, errorString, status); - HandleBinaryOp(this->IsKeyword(keyAND, *argP1) ? (lhs && rhs) - : (lhs || rhs), - arg, newArgs, argP1, argP2); - } + for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; + arg != newArgs.end(); argP1 = ++arg) { + + IncrementArguments(newArgs, argP1, argP2); + + if (argP1 != newArgs.end() && argP2 != newArgs.end() && + (this->IsKeyword(keyAND, *argP1) || this->IsKeyword(keyOR, *argP1))) { + const auto lhs = + this->GetBooleanValueWithAutoDereference(*arg, errorString, status); + const auto rhs = + this->GetBooleanValueWithAutoDereference(*argP2, errorString, status); + HandleBinaryOp(this->IsKeyword(keyAND, *argP1) ? (lhs && rhs) + : (lhs || rhs), + arg, newArgs, argP1, argP2); } - } while (beginSize != newArgs.size()); + } return true; } -- cgit v0.12 From 047f8321a0923d16b97f70e0b3dcfd1eadeddf6f Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 04:13:13 +0300 Subject: Refactor: Avoid redundant `operator<<` on printing messages Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 5f66dbf..5d8dc70 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -193,11 +193,13 @@ cmProp cmConditionEvaluator::GetDefinitionIfUnquoted( this->Policy54Status == cmPolicies::WARN) { if (!this->Makefile.HasCMP0054AlreadyBeenReported(this->Backtrace.Top())) { std::ostringstream e; - e << (cmPolicies::GetPolicyWarning(cmPolicies::CMP0054)) << "\n"; - e << "Quoted variables like \"" << argument.GetValue() - << "\" will no longer be dereferenced " - "when the policy is set to NEW. " + // clang-format off + e << (cmPolicies::GetPolicyWarning(cmPolicies::CMP0054)) + << "\n" + "Quoted variables like \"" << argument.GetValue() << "\" " + "will no longer be dereferenced when the policy is set to NEW. " "Since the policy is not set the OLD behavior will be used."; + // clang-format on this->Makefile.GetCMakeInstance()->IssueMessage( MessageType::AUTHOR_WARNING, e.str(), this->Backtrace); @@ -236,11 +238,14 @@ bool cmConditionEvaluator::IsKeyword(cm::static_string_view keyword, this->Policy54Status == cmPolicies::WARN) { if (!this->Makefile.HasCMP0054AlreadyBeenReported(this->Backtrace.Top())) { std::ostringstream e; - e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0054) << "\n"; - e << "Quoted keywords like \"" << argument.GetValue() - << "\" will no longer be interpreted as keywords " + // clang-format off + e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0054) + << "\n" + "Quoted keywords like \"" << argument.GetValue() << "\" " + "will no longer be interpreted as keywords " "when the policy is set to NEW. " "Since the policy is not set the OLD behavior will be used."; + // clang-format on this->Makefile.GetCMakeInstance()->IssueMessage( MessageType::AUTHOR_WARNING, e.str(), this->Backtrace); @@ -664,8 +669,9 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, HandleBinaryOp(result, arg, newArgs, argP1, argP2); } else if (this->Policy57Status == cmPolicies::WARN) { std::ostringstream e; - e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) << "\n"; - e << "IN_LIST will be interpreted as an operator " + e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) + << "\n" + "IN_LIST will be interpreted as an operator " "when the policy is set to NEW. " "Since the policy is not set the OLD behavior will be used."; -- cgit v0.12 From e3c1dbe18b41752f9e042c6a42170887af6af651 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 05:08:20 +0300 Subject: Refactor: Replace `if` block w/ boolean expression Signed-off-by: Alex Turbov --- Source/cmConditionEvaluator.cxx | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 5d8dc70..3673200 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -657,16 +657,12 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, if (this->Policy57Status != cmPolicies::OLD && this->Policy57Status != cmPolicies::WARN) { - auto result = false; def = this->GetVariableOrString(*arg); def2 = this->Makefile.GetDefinition(argP2->GetValue()); - if (def2) { - result = cm::contains(cmExpandedList(*def2, true), *def); - } - - HandleBinaryOp(result, arg, newArgs, argP1, argP2); + HandleBinaryOp(def2 && cm::contains(cmExpandedList(*def2, true), *def), + arg, newArgs, argP1, argP2); } else if (this->Policy57Status == cmPolicies::WARN) { std::ostringstream e; e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) -- cgit v0.12 From cc6cdacc1860ab14a2980332626b0de826f1eb2b Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 22:12:51 +0300 Subject: Refactor: Simplify boolean to string result assignments --- Source/cmConditionEvaluator.cxx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 3673200..ad2ba3f 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -57,8 +58,6 @@ auto const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"_s; auto const keyVERSION_LESS = "VERSION_LESS"_s; auto const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"_s; -std::array const ZERO_ONE_XLAT = { "0", "1" }; - void IncrementArguments(cmConditionEvaluator::cmArgumentList& newArgs, cmConditionEvaluator::cmArgumentList::iterator& argP1) { @@ -83,7 +82,7 @@ void HandlePredicate(const bool value, cmConditionEvaluator::cmArgumentList& newArgs, cmConditionEvaluator::cmArgumentList::iterator& argP1) { - *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); + *arg = cmExpandedCommandArgument(std::to_string(int(value)), true); newArgs.erase(argP1); argP1 = arg; IncrementArguments(newArgs, argP1); @@ -95,7 +94,7 @@ void HandleBinaryOp(const bool value, cmConditionEvaluator::cmArgumentList::iterator& argP1, cmConditionEvaluator::cmArgumentList::iterator& argP2) { - *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); + *arg = cmExpandedCommandArgument(std::to_string(int(value)), true); newArgs.erase(argP2); newArgs.erase(argP1); argP1 = arg; @@ -383,7 +382,7 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, // now recursively invoke IsTrue to handle the values inside the // parenthetical expression const auto value = this->IsTrue(newArgs2, errorString, status); - *arg = cmExpandedCommandArgument(ZERO_ONE_XLAT[value], true); + *arg = cmExpandedCommandArgument(std::to_string(int(value)), true); argP1 = std::next(arg); // remove the now evaluated parenthetical expression newArgs.erase(argP1, argClose); -- cgit v0.12 From d4d44e86f6a144bc849ffd1439a51a87cbb0cc12 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 22:49:06 +0300 Subject: Refactor: Reduce variables scope in `HandleLevel2` --- Source/cmConditionEvaluator.cxx | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index ad2ba3f..eea7bde 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -503,10 +503,6 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, std::string& errorString, MessageType& status) { - std::string def_buf; - cmProp def; - cmProp def2; - for (auto arg = newArgs.begin(), argP1 = arg, argP2 = arg; arg != newArgs.end(); argP1 = ++arg) { @@ -515,8 +511,9 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, if (argP1 != newArgs.end() && argP2 != newArgs.end() && this->IsKeyword(keyMATCHES, *argP1)) { - def = this->GetDefinitionIfUnquoted(*arg); + cmProp def = this->GetDefinitionIfUnquoted(*arg); + std::string def_buf; if (!def) { def = &arg->GetValue(); } else if (cmHasLiteralPrefix(arg->GetValue(), "CMAKE_MATCH_")) { @@ -526,8 +523,9 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, def = &def_buf; } - const auto& rex = argP2->GetValue(); this->Makefile.ClearMatches(); + + const auto& rex = argP2->GetValue(); cmsys::RegularExpression regEntry; if (!regEntry.compile(rex)) { std::ostringstream error; @@ -568,8 +566,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keyGREATER_EQUAL, *argP1) || this->IsKeyword(keyEQUAL, *argP1)) { - def = this->GetVariableOrString(*arg); - def2 = this->GetVariableOrString(*argP2); + cmProp def = this->GetVariableOrString(*arg); + cmProp def2 = this->GetVariableOrString(*argP2); double lhs; double rhs; @@ -597,8 +595,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keySTRGREATER_EQUAL, *argP1) || this->IsKeyword(keySTREQUAL, *argP1)) { - def = this->GetVariableOrString(*arg); - def2 = this->GetVariableOrString(*argP2); + cmProp def = this->GetVariableOrString(*arg); + cmProp def2 = this->GetVariableOrString(*argP2); const int val = (*def).compare(*def2); bool result; @@ -623,8 +621,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, this->IsKeyword(keyVERSION_GREATER_EQUAL, *argP1) || this->IsKeyword(keyVERSION_EQUAL, *argP1)) { - def = this->GetVariableOrString(*arg); - def2 = this->GetVariableOrString(*argP2); + cmProp def = this->GetVariableOrString(*arg); + cmProp def2 = this->GetVariableOrString(*argP2); cmSystemTools::CompareOp op; if (argP1->GetValue() == keyVERSION_LESS) { @@ -657,8 +655,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, if (this->Policy57Status != cmPolicies::OLD && this->Policy57Status != cmPolicies::WARN) { - def = this->GetVariableOrString(*arg); - def2 = this->Makefile.GetDefinition(argP2->GetValue()); + cmProp def = this->GetVariableOrString(*arg); + cmProp def2 = this->Makefile.GetDefinition(argP2->GetValue()); HandleBinaryOp(def2 && cm::contains(cmExpandedList(*def2, true), *def), arg, newArgs, argP1, argP2); -- cgit v0.12 From 9721ab416f65edd969b756a501b5748d21d4f242 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 23:07:12 +0300 Subject: Refactor: Reorder `MATCHES` handler from top below to the fail-fast --- Source/cmConditionEvaluator.cxx | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index eea7bde..67556ef 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -508,9 +508,22 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, IncrementArguments(newArgs, argP1, argP2); - if (argP1 != newArgs.end() && argP2 != newArgs.end() && - this->IsKeyword(keyMATCHES, *argP1)) { + // NOTE Handle special case `if(... BLAH_BAH MATCHES)` + // (i.e., w/o regex to match which is possibly result of + // variable expansion to an empty string) + if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { + *arg = cmExpandedCommandArgument("0", true); + newArgs.erase(argP1); + argP1 = arg; + IncrementArguments(newArgs, argP1, argP2); + } + // NOTE Fail fast: All the binary ops below require 2 arguments. + else if (argP1 == newArgs.end() || argP2 == newArgs.end()) { + continue; + } + + else if (this->IsKeyword(keyMATCHES, *argP1)) { cmProp def = this->GetDefinitionIfUnquoted(*arg); std::string def_buf; @@ -535,12 +548,11 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, return false; } - if (regEntry.find(*def)) { + const auto match = regEntry.find(*def); + if (match) { this->Makefile.StoreMatches(regEntry); - *arg = cmExpandedCommandArgument("1", true); - } else { - *arg = cmExpandedCommandArgument("0", true); } + *arg = cmExpandedCommandArgument(std::to_string(int(match)), true); newArgs.erase(argP2); newArgs.erase(argP1); @@ -548,18 +560,6 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, IncrementArguments(newArgs, argP1, argP2); } - else if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { - *arg = cmExpandedCommandArgument("0", true); - newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); - } - - // NOTE Fail fast: All the binary ops below require 2 arguments. - else if (argP1 == newArgs.end() || argP2 == newArgs.end()) { - continue; - } - else if (this->IsKeyword(keyLESS, *argP1) || this->IsKeyword(keyLESS_EQUAL, *argP1) || this->IsKeyword(keyGREATER, *argP1) || -- cgit v0.12 From 97d6bbcc01f9e7580ae556d62ee4ceefffbabe3b Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 26 Jul 2021 23:11:56 +0300 Subject: Refactor: Replace `std::to_string` w/ more specialized `bool2string` --- Source/cmConditionEvaluator.cxx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 67556ef..46affaa 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -8,7 +8,6 @@ #include #include #include -#include #include #include @@ -58,6 +57,11 @@ auto const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"_s; auto const keyVERSION_LESS = "VERSION_LESS"_s; auto const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"_s; +std::string bool2string(bool const value) +{ + return std::string(std::size_t(1), static_cast('0' + int(value))); +} + void IncrementArguments(cmConditionEvaluator::cmArgumentList& newArgs, cmConditionEvaluator::cmArgumentList::iterator& argP1) { @@ -82,7 +86,7 @@ void HandlePredicate(const bool value, cmConditionEvaluator::cmArgumentList& newArgs, cmConditionEvaluator::cmArgumentList::iterator& argP1) { - *arg = cmExpandedCommandArgument(std::to_string(int(value)), true); + *arg = cmExpandedCommandArgument(bool2string(value), true); newArgs.erase(argP1); argP1 = arg; IncrementArguments(newArgs, argP1); @@ -94,7 +98,7 @@ void HandleBinaryOp(const bool value, cmConditionEvaluator::cmArgumentList::iterator& argP1, cmConditionEvaluator::cmArgumentList::iterator& argP2) { - *arg = cmExpandedCommandArgument(std::to_string(int(value)), true); + *arg = cmExpandedCommandArgument(bool2string(value), true); newArgs.erase(argP2); newArgs.erase(argP1); argP1 = arg; @@ -382,7 +386,7 @@ bool cmConditionEvaluator::HandleLevel0(cmArgumentList& newArgs, // now recursively invoke IsTrue to handle the values inside the // parenthetical expression const auto value = this->IsTrue(newArgs2, errorString, status); - *arg = cmExpandedCommandArgument(std::to_string(int(value)), true); + *arg = cmExpandedCommandArgument(bool2string(value), true); argP1 = std::next(arg); // remove the now evaluated parenthetical expression newArgs.erase(argP1, argClose); @@ -552,7 +556,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, if (match) { this->Makefile.StoreMatches(regEntry); } - *arg = cmExpandedCommandArgument(std::to_string(int(match)), true); + *arg = cmExpandedCommandArgument(bool2string(match), true); newArgs.erase(argP2); newArgs.erase(argP1); -- cgit v0.12 From 2b916606c5c3f1145edd5f0464b139d03b79ae44 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Tue, 27 Jul 2021 00:13:21 +0300 Subject: Refactor: No need to move iterators after match A handler's loop of all levels gonna restart after calls to `HandlePredcate` or `HandleBinaryOp`... And the first action in the loop is to setup iterators. So, no need to move 'em inside the functions. Also, no need to pass iterators by reference. Also, reorder parameters of both functions so iterators followed after a reference to the newArgs container. --- Source/cmConditionEvaluator.cxx | 68 +++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 40 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 46affaa..38dfca5 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -82,27 +82,23 @@ void IncrementArguments(cmConditionEvaluator::cmArgumentList& newArgs, } void HandlePredicate(const bool value, - cmConditionEvaluator::cmArgumentList::iterator& arg, cmConditionEvaluator::cmArgumentList& newArgs, - cmConditionEvaluator::cmArgumentList::iterator& argP1) + cmConditionEvaluator::cmArgumentList::iterator arg, + cmConditionEvaluator::cmArgumentList::iterator argP1) { *arg = cmExpandedCommandArgument(bool2string(value), true); newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1); } void HandleBinaryOp(const bool value, - cmConditionEvaluator::cmArgumentList::iterator& arg, cmConditionEvaluator::cmArgumentList& newArgs, - cmConditionEvaluator::cmArgumentList::iterator& argP1, - cmConditionEvaluator::cmArgumentList::iterator& argP2) + cmConditionEvaluator::cmArgumentList::iterator arg, + cmConditionEvaluator::cmArgumentList::iterator argP1, + cmConditionEvaluator::cmArgumentList::iterator argP2) { *arg = cmExpandedCommandArgument(bool2string(value), true); newArgs.erase(argP2); newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); } } // anonymous namespace @@ -435,41 +431,41 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, // does a file exist if (this->IsKeyword(keyEXISTS, *arg)) { - HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), arg, - newArgs, argP1); + HandlePredicate(cmSystemTools::FileExists(argP1->GetValue()), newArgs, + arg, argP1); } // does a directory with this name exist else if (this->IsKeyword(keyIS_DIRECTORY, *arg)) { - HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), arg, - newArgs, argP1); + HandlePredicate(cmSystemTools::FileIsDirectory(argP1->GetValue()), + newArgs, arg, argP1); } // does a symlink with this name exist else if (this->IsKeyword(keyIS_SYMLINK, *arg)) { - HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), arg, - newArgs, argP1); + HandlePredicate(cmSystemTools::FileIsSymlink(argP1->GetValue()), newArgs, + arg, argP1); } // is the given path an absolute path ? else if (this->IsKeyword(keyIS_ABSOLUTE, *arg)) { - HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), arg, - newArgs, argP1); + HandlePredicate(cmSystemTools::FileIsFullPath(argP1->GetValue()), + newArgs, arg, argP1); } // does a command exist else if (this->IsKeyword(keyCOMMAND, *arg)) { HandlePredicate( this->Makefile.GetState()->GetCommand(argP1->GetValue()) != nullptr, - arg, newArgs, argP1); + newArgs, arg, argP1); } // does a policy exist else if (this->IsKeyword(keyPOLICY, *arg)) { cmPolicies::PolicyID pid; HandlePredicate(cmPolicies::GetPolicyID(argP1->GetValue().c_str(), pid), - arg, newArgs, argP1); + newArgs, arg, argP1); } // does a target exist else if (this->IsKeyword(keyTARGET, *arg)) { HandlePredicate(this->Makefile.FindTargetToUse(argP1->GetValue()) != nullptr, - arg, newArgs, argP1); + newArgs, arg, argP1); } // is a variable defined else if (this->IsKeyword(keyDEFINED, *arg)) { @@ -487,7 +483,7 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, } else { bdef = this->Makefile.IsDefinitionSet(argP1->GetValue()); } - HandlePredicate(bdef, arg, newArgs, argP1); + HandlePredicate(bdef, newArgs, arg, argP1); } // does a test exist else if (this->IsKeyword(keyTEST, *arg)) { @@ -495,7 +491,7 @@ bool cmConditionEvaluator::HandleLevel1(cmArgumentList& newArgs, std::string&, continue; } HandlePredicate(this->Makefile.GetTest(argP1->GetValue()) != nullptr, - arg, newArgs, argP1); + newArgs, arg, argP1); } } return true; @@ -516,10 +512,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, // (i.e., w/o regex to match which is possibly result of // variable expansion to an empty string) if (argP1 != newArgs.end() && this->IsKeyword(keyMATCHES, *arg)) { - *arg = cmExpandedCommandArgument("0", true); - newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); + HandlePredicate(false, newArgs, arg, argP1); } // NOTE Fail fast: All the binary ops below require 2 arguments. @@ -556,12 +549,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, if (match) { this->Makefile.StoreMatches(regEntry); } - *arg = cmExpandedCommandArgument(bool2string(match), true); - - newArgs.erase(argP2); - newArgs.erase(argP1); - argP1 = arg; - IncrementArguments(newArgs, argP1, argP2); + HandleBinaryOp(match, newArgs, arg, argP1, argP2); } else if (this->IsKeyword(keyLESS, *argP1) || @@ -590,7 +578,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } else { result = (lhs == rhs); } - HandleBinaryOp(result, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, newArgs, arg, argP1, argP2); } else if (this->IsKeyword(keySTRLESS, *argP1) || @@ -616,7 +604,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, { result = (val == 0); } - HandleBinaryOp(result, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, newArgs, arg, argP1, argP2); } else if (this->IsKeyword(keyVERSION_LESS, *argP1) || @@ -642,7 +630,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, } const auto result = cmSystemTools::VersionCompare(op, def->c_str(), def2->c_str()); - HandleBinaryOp(result, arg, newArgs, argP1, argP2); + HandleBinaryOp(result, newArgs, arg, argP1, argP2); } // is file A newer than file B @@ -650,8 +638,8 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, auto fileIsNewer = 0; cmsys::Status ftcStatus = cmSystemTools::FileTimeCompare( arg->GetValue(), argP2->GetValue(), &fileIsNewer); - HandleBinaryOp((!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), arg, - newArgs, argP1, argP2); + HandleBinaryOp((!ftcStatus || fileIsNewer == 1 || fileIsNewer == 0), + newArgs, arg, argP1, argP2); } else if (this->IsKeyword(keyIN_LIST, *argP1)) { @@ -663,7 +651,7 @@ bool cmConditionEvaluator::HandleLevel2(cmArgumentList& newArgs, cmProp def2 = this->Makefile.GetDefinition(argP2->GetValue()); HandleBinaryOp(def2 && cm::contains(cmExpandedList(*def2, true), *def), - arg, newArgs, argP1, argP2); + newArgs, arg, argP1, argP2); } else if (this->Policy57Status == cmPolicies::WARN) { std::ostringstream e; e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0057) @@ -693,7 +681,7 @@ bool cmConditionEvaluator::HandleLevel3(cmArgumentList& newArgs, if (argP1 != newArgs.end() && this->IsKeyword(keyNOT, *arg)) { const auto rhs = this->GetBooleanValueWithAutoDereference(*argP1, errorString, status); - HandlePredicate(!rhs, arg, newArgs, argP1); + HandlePredicate(!rhs, newArgs, arg, argP1); } } return true; @@ -718,7 +706,7 @@ bool cmConditionEvaluator::HandleLevel4(cmArgumentList& newArgs, this->GetBooleanValueWithAutoDereference(*argP2, errorString, status); HandleBinaryOp(this->IsKeyword(keyAND, *argP1) ? (lhs && rhs) : (lhs || rhs), - arg, newArgs, argP1, argP2); + newArgs, arg, argP1, argP2); } } return true; -- cgit v0.12 From 17af3baddd7b7056114748f16e63b8de6b3f67e8 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Tue, 27 Jul 2021 05:33:53 +0300 Subject: Refactor: Set `const` to `cmConditionEvaluator::IsKeyword` parameters --- Source/cmConditionEvaluator.cxx | 5 +++-- Source/cmConditionEvaluator.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 38dfca5..cec9691 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -222,8 +222,9 @@ cmProp cmConditionEvaluator::GetVariableOrString( } //========================================================================= -bool cmConditionEvaluator::IsKeyword(cm::static_string_view keyword, - cmExpandedCommandArgument& argument) const +bool cmConditionEvaluator::IsKeyword( + cm::static_string_view keyword, + const cmExpandedCommandArgument& argument) const { if ((this->Policy54Status != cmPolicies::WARN && this->Policy54Status != cmPolicies::OLD) && diff --git a/Source/cmConditionEvaluator.h b/Source/cmConditionEvaluator.h index a7e3ce8..d74e28b 100644 --- a/Source/cmConditionEvaluator.h +++ b/Source/cmConditionEvaluator.h @@ -39,7 +39,7 @@ private: cmProp GetVariableOrString(const cmExpandedCommandArgument& argument) const; bool IsKeyword(cm::static_string_view keyword, - cmExpandedCommandArgument& argument) const; + const cmExpandedCommandArgument& argument) const; bool GetBooleanValue(cmExpandedCommandArgument& arg) const; -- cgit v0.12 From 46810235e3a8420b3379b352dd6b19d7a0439acd Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Tue, 27 Jul 2021 06:35:54 +0300 Subject: =?UTF-8?q?Refactor:=20Avoid=20`if`=20=E2=86=92=20`else=20if`=20?= =?UTF-8?q?=E2=86=92=20=E2=80=A6=20for=20compare=20operators?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When trying to match one of compare operator key inside `if()` condition, remember the index of matched operator. Later this index used to select the operation to perform instead of strings compare again. --- Source/cmConditionEvaluator.cxx | 157 ++++++++++++++++++++++++---------------- Source/cmConditionEvaluator.h | 9 +++ 2 files changed, 104 insertions(+), 62 deletions(-) diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index cec9691..570bb4a 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/Source/cmConditionEvaluator.cxx @@ -57,6 +57,34 @@ auto const keyVERSION_GREATER_EQUAL = "VERSION_GREATER_EQUAL"_s; auto const keyVERSION_LESS = "VERSION_LESS"_s; auto const keyVERSION_LESS_EQUAL = "VERSION_LESS_EQUAL"_s; +// Run-Time to Compile-Time template selector +template