From ae235b1ef5a59da3bb5f4e2a83f40ab8072a0598 Mon Sep 17 00:00:00 2001 From: Steve Plimpton Date: Tue, 28 Jun 2022 16:19:06 -0600 Subject: [PATCH 1/7] Boolean expression corner case --- src/variable.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/variable.cpp b/src/variable.cpp index 7392379017..bcb42f0599 100644 --- a/src/variable.cpp +++ b/src/variable.cpp @@ -4775,7 +4775,7 @@ double Variable::evaluate_boolean(char *str) else argstack[nargstack].value = 0.0; } else if (opprevious == EQ) { if (flag1 != flag2) - error->all(FLERR,"Invalid Boolean syntax in if command"); + error->all(FLERR,"If command boolean comparing string to number"); if (flag2 == 0) { if (value1 == value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; @@ -4845,6 +4845,10 @@ double Variable::evaluate_boolean(char *str) if (nopstack) error->all(FLERR,"Invalid Boolean syntax in if command"); if (nargstack != 1) error->all(FLERR,"Invalid Boolean syntax in if command"); + + // if flag == 1, Boolean expression was a single string with no operator + + if (argstack[0].flag == 1) return 0.0; return argstack[0].value; } From e4e9b2e49ada676f19ffc63f962e4fbaa10ef940 Mon Sep 17 00:00:00 2001 From: Steve Plimpton Date: Tue, 28 Jun 2022 16:37:04 -0600 Subject: [PATCH 2/7] more consistency checks --- doc/src/if.rst | 32 ++++++++++++++++++++------------ src/variable.cpp | 31 ++++++++++++++++++++----------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/doc/src/if.rst b/doc/src/if.rst index 08ec6f2c3e..2125e2f9a9 100644 --- a/doc/src/if.rst +++ b/doc/src/if.rst @@ -156,27 +156,28 @@ and Boolean operators: Each A and B is a number or string or a variable reference like $a or ${abc}, or A or B can be another Boolean expression. -If a variable is used it can produce a number when evaluated, like an -:doc:`equal-style variable `. Or it can produce a string, -like an :doc:`index-style variable `. For an individual -Boolean operator, A and B must both be numbers or must both be -strings. You cannot compare a number to a string. +Note that all variables used will be substituted for before the +Boolean expression in evaluated. A variable can produce a number, +like an :doc:`equal-style variable `. Or it can produce a +string, like an :doc:`index-style variable `. + +The Boolean operators "==" and "!=" can operate on a pair or strings +or numbers. They cannot compare a number to a string. All the other +Boolean operations can only operate on numbers. Expressions are evaluated left to right and have the usual C-style precedence: the unary logical NOT operator "!" has the highest precedence, the 4 relational operators "<", "<=", ">", and ">=" are next; the two remaining relational operators "==" and "!=" are next; then the logical AND operator "&&"; and finally the logical OR -operator "\|\|" and logical XOR (exclusive or) operator "\|\^" have the -lowest precedence. Parenthesis can be used to group one or more +operator "\|\|" and logical XOR (exclusive or) operator "\|\^" have +the lowest precedence. Parenthesis can be used to group one or more portions of an expression and/or enforce a different order of evaluation than what would occur with the default precedence. When the 6 relational operators (first 6 in list above) compare 2 numbers, they return either a 1.0 or 0.0 depending on whether the -relationship between A and B is TRUE or FALSE. When the 6 relational -operators compare 2 strings, they also return a 1.0 or 0.0 for TRUE or -FALSE, but the comparison is done by the C function strcmp(). +relationship between A and B is TRUE or FALSE. When the 3 logical operators (last 3 in list above) compare 2 numbers, they also return either a 1.0 or 0.0 depending on whether the @@ -190,8 +191,15 @@ returns 1.0 if its argument is 0.0, else it returns 0.0. The 3 logical operators can only be used to operate on numbers, not on strings. -The overall Boolean expression produces a TRUE result if the result is -non-zero. If the result is zero, the expression result is FALSE. +The overall Boolean expression produces a TRUE result if the numeric +result is non-zero. If the result is zero, the expression result is +FALSE. + +.. note:: + + If the Boolean expression is a single numeric value with no Boolean + operators, it will be FALSE if the value = 0.0, otherwise TRUE. If the + Boolean expression is a single string, it will always be FALSE. ---------- diff --git a/src/variable.cpp b/src/variable.cpp index bcb42f0599..9afc4d5c6a 100644 --- a/src/variable.cpp +++ b/src/variable.cpp @@ -4770,12 +4770,14 @@ double Variable::evaluate_boolean(char *str) } if (opprevious == NOT) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); + if (flag2) + error->all(FLERR,"If command boolean not cannot operate on string"); if (value2 == 0.0) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; + } else if (opprevious == EQ) { if (flag1 != flag2) - error->all(FLERR,"If command boolean comparing string to number"); + error->all(FLERR,"If command boolean is comparing string to number"); if (flag2 == 0) { if (value1 == value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; @@ -4787,7 +4789,7 @@ double Variable::evaluate_boolean(char *str) } } else if (opprevious == NE) { if (flag1 != flag2) - error->all(FLERR,"Invalid Boolean syntax in if command"); + error->all(FLERR,"If command boolean is comparing string to number"); if (flag2 == 0) { if (value1 != value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; @@ -4797,32 +4799,39 @@ double Variable::evaluate_boolean(char *str) delete[] str1; delete[] str2; } + } else if (opprevious == LT) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); - if (value1 < value2) argstack[nargstack].value = 1.0; + if (flag1 || flag2) + error->all(FLERR,"If command boolean can only operate on numbers"); else argstack[nargstack].value = 0.0; } else if (opprevious == LE) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); + if (flag1 || flag2) + error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 <= value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == GT) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); + if (flag1 || flag2) + error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 > value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == GE) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); + if (flag1 || flag2) + error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 >= value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == AND) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); + if (flag1 || flag2) + error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 != 0.0 && value2 != 0.0) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == OR) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); + if (flag1 || flag2) + error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 != 0.0 || value2 != 0.0) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == XOR) { - if (flag2) error->all(FLERR,"Invalid Boolean syntax in if command"); + if (flag1 || flag2) + error->all(FLERR,"If command boolean can only operate on numbers"); if ((value1 == 0.0 && value2 != 0.0) || (value1 != 0.0 && value2 == 0.0)) argstack[nargstack].value = 1.0; From 1dc71ef3e344beee9aa861babc9c21d237e23313 Mon Sep 17 00:00:00 2001 From: Steve Plimpton Date: Tue, 28 Jun 2022 16:38:51 -0600 Subject: [PATCH 3/7] better error strings --- src/variable.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/variable.cpp b/src/variable.cpp index 9afc4d5c6a..502d4d3312 100644 --- a/src/variable.cpp +++ b/src/variable.cpp @@ -4819,6 +4819,7 @@ double Variable::evaluate_boolean(char *str) error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 >= value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; + } else if (opprevious == AND) { if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); From 1fabd5a56b62637d9f7070956952611689229b3b Mon Sep 17 00:00:00 2001 From: Steve Plimpton Date: Wed, 29 Jun 2022 07:49:22 -0600 Subject: [PATCH 4/7] change boolean = single string to an error --- doc/src/if.rst | 5 +++-- src/variable.cpp | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/doc/src/if.rst b/doc/src/if.rst index 2125e2f9a9..91b001fbdc 100644 --- a/doc/src/if.rst +++ b/doc/src/if.rst @@ -198,8 +198,9 @@ FALSE. .. note:: If the Boolean expression is a single numeric value with no Boolean - operators, it will be FALSE if the value = 0.0, otherwise TRUE. If the - Boolean expression is a single string, it will always be FALSE. + operators, it will be FALSE if the value = 0.0, otherwise TRUE. If + the Boolean expression is a single string, an error message will be + issued. ---------- diff --git a/src/variable.cpp b/src/variable.cpp index 502d4d3312..31187f69dc 100644 --- a/src/variable.cpp +++ b/src/variable.cpp @@ -4857,8 +4857,11 @@ double Variable::evaluate_boolean(char *str) if (nargstack != 1) error->all(FLERR,"Invalid Boolean syntax in if command"); // if flag == 1, Boolean expression was a single string with no operator + // error b/c invalid, only single number with no operator is allowed + + if (argstack[0].flag == 1) + error->all(FLERR,"If command boolean cannot be single string"); - if (argstack[0].flag == 1) return 0.0; return argstack[0].value; } From 793069d8eb240c028578ef8e6c508dc09dddda79 Mon Sep 17 00:00:00 2001 From: Axel Kohlmeyer Date: Wed, 29 Jun 2022 12:24:43 -0400 Subject: [PATCH 5/7] update and expand unit tests for if() command boolean evaluation --- unittest/commands/test_variables.cpp | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/unittest/commands/test_variables.cpp b/unittest/commands/test_variables.cpp index 0533de7329..35bab69c7d 100644 --- a/unittest/commands/test_variables.cpp +++ b/unittest/commands/test_variables.cpp @@ -389,6 +389,8 @@ TEST_F(VariableTest, IfCommand) { BEGIN_HIDE_OUTPUT(); command("variable one index 1"); + command("variable two string xx"); + command("variable three equal 1"); END_HIDE_OUTPUT(); BEGIN_CAPTURE_OUTPUT(); @@ -444,7 +446,11 @@ TEST_F(VariableTest, IfCommand) BEGIN_CAPTURE_OUTPUT(); command("if x!=x|^a!=b then 'print \"bingo!\"'"); text = END_CAPTURE_OUTPUT(); + ASSERT_THAT(text, ContainsRegex(".*bingo!.*")); + BEGIN_CAPTURE_OUTPUT(); + command("if (${three}) then 'print \"bingo!\"'"); + text = END_CAPTURE_OUTPUT(); ASSERT_THAT(text, ContainsRegex(".*bingo!.*")); TEST_FAILURE(".*ERROR: Invalid Boolean syntax in if command.*", @@ -455,8 +461,16 @@ TEST_F(VariableTest, IfCommand) command("if 1a then 'print \"bingo!\"'");); TEST_FAILURE(".*ERROR: Invalid Boolean syntax in if command.*", command("if 1=<2 then 'print \"bingo!\"'");); - TEST_FAILURE(".*ERROR: Invalid Boolean syntax in if command.*", + TEST_FAILURE(".*ERROR: If command boolean is comparing string to number.*", command("if 1!=a then 'print \"bingo!\"'");); + TEST_FAILURE(".*ERROR: If command boolean can only operate on numbers.*", + command("if ab then 'print \"bingo!\"'");); + TEST_FAILURE(".*ERROR: If command boolean can only operate on numbers.*", + command("if a<=b then 'print \"bingo!\"'");); + TEST_FAILURE(".*ERROR: If command boolean can only operate on numbers.*", + command("if a<=b then 'print \"bingo!\"'");); TEST_FAILURE(".*ERROR: Invalid Boolean syntax in if command.*", command("if 1&<2 then 'print \"bingo!\"'");); TEST_FAILURE(".*ERROR: Invalid Boolean syntax in if command.*", @@ -465,8 +479,14 @@ TEST_F(VariableTest, IfCommand) command("if (1)( then 'print \"bingo!\"'");); TEST_FAILURE(".*ERROR: Invalid Boolean syntax in if command.*", command("if (1)1 then 'print \"bingo!\"'");); - TEST_FAILURE(".*ERROR: Invalid Boolean syntax in if command.*", + TEST_FAILURE(".*ERROR: If command boolean is comparing string to number.*", command("if (v_one==1.0)&&(2>=1) then 'print \"bingo!\"'");); + TEST_FAILURE(".*ERROR: If command boolean cannot be single string.*", + command("if (something) then 'print \"bingo!\"'");); + TEST_FAILURE(".*ERROR: If command boolean cannot be single string.*", + command("if (v_one) then 'print \"bingo!\"'");); + TEST_FAILURE(".*ERROR: If command boolean cannot be single string.*", + command("if (${two}) then 'print \"bingo!\"'");); } TEST_F(VariableTest, NextCommand) From 137dc6243e6817058698f0e1cec3f61172ad1f24 Mon Sep 17 00:00:00 2001 From: Axel Kohlmeyer Date: Wed, 29 Jun 2022 12:25:15 -0400 Subject: [PATCH 6/7] whitespace --- src/variable.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/variable.cpp b/src/variable.cpp index 31187f69dc..1c94356186 100644 --- a/src/variable.cpp +++ b/src/variable.cpp @@ -4770,7 +4770,7 @@ double Variable::evaluate_boolean(char *str) } if (opprevious == NOT) { - if (flag2) + if (flag2) error->all(FLERR,"If command boolean not cannot operate on string"); if (value2 == 0.0) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; @@ -4801,37 +4801,37 @@ double Variable::evaluate_boolean(char *str) } } else if (opprevious == LT) { - if (flag1 || flag2) + if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); else argstack[nargstack].value = 0.0; } else if (opprevious == LE) { - if (flag1 || flag2) + if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 <= value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == GT) { - if (flag1 || flag2) + if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 > value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == GE) { - if (flag1 || flag2) + if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 >= value2) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == AND) { - if (flag1 || flag2) + if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 != 0.0 && value2 != 0.0) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == OR) { - if (flag1 || flag2) + if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); if (value1 != 0.0 || value2 != 0.0) argstack[nargstack].value = 1.0; else argstack[nargstack].value = 0.0; } else if (opprevious == XOR) { - if (flag1 || flag2) + if (flag1 || flag2) error->all(FLERR,"If command boolean can only operate on numbers"); if ((value1 == 0.0 && value2 != 0.0) || (value1 != 0.0 && value2 == 0.0)) @@ -4859,7 +4859,7 @@ double Variable::evaluate_boolean(char *str) // if flag == 1, Boolean expression was a single string with no operator // error b/c invalid, only single number with no operator is allowed - if (argstack[0].flag == 1) + if (argstack[0].flag == 1) error->all(FLERR,"If command boolean cannot be single string"); return argstack[0].value; From f273a28a2a625618c959890395479ca5c339f0a9 Mon Sep 17 00:00:00 2001 From: Axel Kohlmeyer Date: Wed, 29 Jun 2022 12:26:16 -0400 Subject: [PATCH 7/7] whitespace --- src/KOKKOS/min_linesearch_kokkos.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/KOKKOS/min_linesearch_kokkos.cpp b/src/KOKKOS/min_linesearch_kokkos.cpp index cb97a34e50..76ea522fa8 100644 --- a/src/KOKKOS/min_linesearch_kokkos.cpp +++ b/src/KOKKOS/min_linesearch_kokkos.cpp @@ -363,7 +363,7 @@ double MinLineSearchKokkos::alpha_step(double alpha, int resetflag) // reset to starting point if (nextra_global) modify->min_step(0.0,hextra); - atomKK->k_x.clear_sync_state(); // ignore if host positions modified since + atomKK->k_x.clear_sync_state(); // ignore if host positions modified since // device positions will be reset below { // local variables for lambda capture