Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make conversion of Option to bool stricter #2282

Merged
merged 3 commits into from
Apr 15, 2021
Merged

Conversation

johnomotani
Copy link
Contributor

Previously, Options::as<bool>() only tested (case-insensitively) if first character was 'n', 'f', '0', 'y', 't', or '1'. Now only allow (still case-insensitively but checking full strings) 'n', 'no', 'f', 'false', '0', 'y', 'yes', 't', 'true', or '1'. See discussion on boutproject/boutdata#32.

I found a nice intro to parameterised tests here https://www.sandordargo.com/blog/2019/04/24/parameterized-testing-with-gtest, which I used for the unit tests.

Previously, only tested (case-insensitively) if first character was 'n',
'f', '0', 'y', 't', or '1'. Now only allow (still case-insensitively but
checking full strings) 'n', 'no', 'f', 'false', '0', 'y', 'yes', 't',
'true', or '1'.
@johnomotani johnomotani added the breaking change Introduces a change that is not backward compatible with the previous major version label Apr 14, 2021
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@ZedThree
Copy link
Member

Tests are failing because we check the old behaviour:

// Surprise true
options.set("bool_key3", "yes_this_is_a_bool", "code2");
EXPECT_NO_THROW(options.get("bool_key3", value3, false, false));
EXPECT_EQ(value3, true);

Deleting those lines should be sufficient

@ZedThree ZedThree added this to the BOUT-5.0 milestone Apr 14, 2021
src/sys/options.cxx Outdated Show resolved Hide resolved

auto c = static_cast<char>(toupper((strvalue)[0]));
if ((c == 'Y') || (c == 'T') || (c == '1')) {
if ((strvalue == "y") or (strvalue == "yes") or (strvalue == "t")
Copy link
Member

Choose a reason for hiding this comment

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

Just for my own curiosity, I had a stab at rewriting this to avoid the long-ish chain of comparisons, but I think this is probably worse:

auto one_of = [&strvalue](const std::vector<std::string>& values) -> bool {
  return std::any_of(
      values.begin(), values.end(),
      [&strvalue](const auto& value) { return value == strvalue; });
};
if (one_of({"y", "yes", "t", "true", "1"})) {
  result = true;
} else if (one_of({"n", "no", "f", "false", "0"})) {
  result = false;
} else {

Could be worth if it one_of were a function we could reuse somewhere else.

I think it just betrays my love of lambdas and C++'s love of verbose grammar. Here it is in Python:

if strvalue.lower() in ["y", "yes", "t", "true", "1"]:
  result = True
elif if strvalue.lower() in ["n", "no", "f", "false", "0"]:
   result = False
else:
   raise ValueError()

@ZedThree
Copy link
Member

Looks good, thanks @johnomotani !

@ZedThree ZedThree merged commit 1d1307a into next Apr 15, 2021
@ZedThree ZedThree deleted the stricter-bool-conversion branch April 15, 2021 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Introduces a change that is not backward compatible with the previous major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants