Avoid exception in Options when parsing string to Field #2457

dschwoerer · 2021-10-28T13:06:12Z

Mostly annoying with debugging, if an expected exception is thrown.

github-actions

clang-tidy made some suggestions

src/sys/options.cxx

github-actions · 2021-10-28T13:11:09Z

src/sys/options.cxx

+    return FieldFactory::get()->create3D(bout::utils::get<std::string>(value), this,
+                                         similar_to.getMesh(), similar_to.getLocation());
+  } else if (bout::utils::holds_alternative<Tensor<BoutReal>>(value)) {
+    auto localmesh = similar_to.getMesh();


warning: auto localmesh can be declared as auto *localmesh [readability-qualified-auto]

auto localmesh = similar_to.getMesh(); ^ note: this fix will not be applied because it overlaps with another fix

I am wondering about this one - wouldn't it increase readability even more if this would be Mesh* rather than auto*?

Yes, you're probably right -- this bit isn't generic, the type isn't already repeated on the left, and we're unlikely to change the type, so Mesh* is probably good

github-actions · 2021-10-28T13:11:09Z

src/sys/options.cxx

+                                         similar_to.getMesh(), similar_to.getLocation());
+  } else if (bout::utils::holds_alternative<Tensor<BoutReal>>(value)) {
+    auto localmesh = similar_to.getMesh();
+    if (!localmesh) {


warning: implicit conversion Mesh * -> bool [readability-implicit-bool-conversion]

if (!localmesh) { ^ note: this fix will not be applied because it overlaps with another fix

ZedThree · 2021-10-28T13:18:02Z

While you're touching this, please could you do the same for as<Field2D> and as<FieldPerp>? 🙂

github-actions

clang-tidy made some suggestions

github-actions · 2021-10-28T13:33:34Z

src/sys/options.cxx

+                                         similar_to.getMesh(), similar_to.getLocation());
+  }
+  if (bout::utils::holds_alternative<Tensor<BoutReal>>(value)) {
+    auto* localmesh = similar_to.getMesh();


warning: auto localmesh can be declared as auto *localmesh [readability-qualified-auto]

auto localmesh = similar_to.getMesh(); ^~~~~ auto *

github-actions · 2021-10-28T13:33:34Z

src/sys/options.cxx

+  }
+  if (bout::utils::holds_alternative<Tensor<BoutReal>>(value)) {
+    auto* localmesh = similar_to.getMesh();
+    if (localmesh != nullptr) {


warning: implicit conversion Mesh * -> bool [readability-implicit-bool-conversion]

if (!localmesh) { ~^ == nullptr

Suggested change

if (localmesh != nullptr) {

if (localmesh == nullptr) {

github-actions · 2021-10-28T13:33:34Z

src/sys/options.cxx

+                                         similar_to.getMesh(), similar_to.getLocation());
+  }
+  if (bout::utils::holds_alternative<Matrix<BoutReal>>(value)) {
+    auto* localmesh = similar_to.getMesh();


warning: auto localmesh can be declared as auto *localmesh [readability-qualified-auto]

auto localmesh = similar_to.getMesh(); ^~~~~ auto *

github-actions · 2021-10-28T13:33:35Z

src/sys/options.cxx

+  }
+  if (bout::utils::holds_alternative<Matrix<BoutReal>>(value)) {
+    auto* localmesh = similar_to.getMesh();
+    if (localmesh != nullptr) {


warning: implicit conversion Mesh * -> bool [readability-implicit-bool-conversion]

if (!localmesh) { ~^ == nullptr

Suggested change

if (localmesh != nullptr) {

if (localmesh == nullptr) {

github-actions · 2021-10-28T13:33:35Z

src/sys/options.cxx

+                                           similar_to.getMesh(), location);
+  }
+  if (bout::utils::holds_alternative<Matrix<BoutReal>>(value)) {
+    auto* localmesh = similar_to.getMesh();


warning: auto localmesh can be declared as auto *localmesh [readability-qualified-auto]

auto localmesh = similar_to.getMesh(); ^~~~~ auto *

github-actions · 2021-10-28T13:33:35Z

src/sys/options.cxx

+  }
+  if (bout::utils::holds_alternative<Matrix<BoutReal>>(value)) {
+    auto* localmesh = similar_to.getMesh();
+    if (localmesh != nullptr) {


warning: implicit conversion Mesh * -> bool [readability-implicit-bool-conversion]

if (!localmesh) { ~^ == nullptr

I thought the huge long comment from clang-tidy below was noise, but it actually caught the typo here!

Suggested change

if (localmesh != nullptr) {

if (localmesh == nullptr) {

Good catch, I thought the analysis was invalid as I thought the last two steps would contradict each other - but you are right 👍

github-actions

clang-tidy made some suggestions

src/sys/options.cxx

github-actions · 2021-10-28T14:07:19Z

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

ZedThree · 2021-10-28T14:13:37Z

Tests are failing because we also need to handle the case where the variant holds an int. We probably want:

  if (bout::utils::holds_alternative<BoutReal>(value) or bout::utils::holds_alternative<int>(value)) {
    BoutReal scalar_value = bout::utils::variantStaticCastOrThrow<ValueType, BoutReal>(value);

    // Get metadata from similar_to, fill field with scalar_value
    return filledFrom(similar_to, scalar_value);
  }

or alternatively:

  if (bout::utils::holds_alternative<BoutReal>(value)) {
    BoutReal scalar_value = bout::utils::get<BoutReal>(value);

    // Get metadata from similar_to, fill field with scalar_value
    return filledFrom(similar_to, scalar_value);
  }
  if (bout::utils::holds_alternative<int>(value)) {
    BoutReal scalar_value = bout::utils::get<int>(value);

    // Get metadata from similar_to, fill field with scalar_value
    return filledFrom(similar_to, scalar_value);
  }

shorter version:

  if (bout::utils::holds_alternative<BoutReal>(value)) {
    // Get metadata from similar_to, fill field with scalar_value
    return filledFrom(similar_to, bout::utils::get<BoutReal>(value));
  }
  if (bout::utils::holds_alternative<int>(value)) {
    // Get metadata from similar_to, fill field with scalar_value
    return filledFrom(similar_to, bout::utils::get<int>(value));
  }

ZedThree · 2021-10-28T15:19:18Z

Something seems to be making the Optimised, shared, Python (pull_request) build hang -- but not the (push) version. Currently running for 57 minutes. I'm guessing it'll get killed soon and then maybe we'll be able to see the logs

dschwoerer · 2021-10-28T15:30:00Z

Unfortunately the logs do not reveal anything. Unless you have a better idea, I would just restart to see whether it is reproducible ...

ZedThree · 2021-10-28T15:36:05Z

Looking at the timings in the logs, it looks like most things just took 10 minutes longer than in the push build for some reason. Everything ran fine though. Unfortunately, it's not possible to restart just one job, it will need to restart all of the (pull_request) jobs.

I'm going to use admin powers to merge this, the tests will get run again on the merge anyway.

Avoid exception in options

3d1e18c

Mostly annoying with debugging, if an expected exception is thrown.

github-actions bot reviewed Oct 28, 2021

View reviewed changes

dschwoerer added 2 commits October 28, 2021 15:25

Remove try statements from other fields

ddc0f7c

clang-tidy fixes

67ea21e

github-actions bot reviewed Oct 28, 2021

View reviewed changes

src/sys/options.cxx Show resolved Hide resolved

src/sys/options.cxx Show resolved Hide resolved

src/sys/options.cxx Show resolved Hide resolved

Fix typo

0491cad

Accept both float and int

19d3f99

ZedThree approved these changes Oct 28, 2021

View reviewed changes

ZedThree changed the title ~~Avoid exception in options~~ Avoid exception in Options when parsing string to Field Oct 28, 2021

ZedThree merged commit 024111e into next Oct 28, 2021

ZedThree deleted the clean-init branch October 28, 2021 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid exception in Options when parsing string to Field #2457

Avoid exception in Options when parsing string to Field #2457

dschwoerer commented Oct 28, 2021

github-actions bot left a comment

github-actions bot Oct 28, 2021

dschwoerer Oct 28, 2021

ZedThree Oct 28, 2021

github-actions bot Oct 28, 2021

ZedThree commented Oct 28, 2021

github-actions bot left a comment

github-actions bot Oct 28, 2021

github-actions bot Oct 28, 2021

ZedThree Oct 28, 2021

github-actions bot Oct 28, 2021

github-actions bot Oct 28, 2021

ZedThree Oct 28, 2021

github-actions bot Oct 28, 2021

github-actions bot Oct 28, 2021

ZedThree Oct 28, 2021

dschwoerer Oct 28, 2021

github-actions bot left a comment

github-actions bot commented Oct 28, 2021

ZedThree commented Oct 28, 2021

ZedThree commented Oct 28, 2021

dschwoerer commented Oct 28, 2021

ZedThree commented Oct 28, 2021

Avoid exception in Options when parsing string to Field #2457

Avoid exception in Options when parsing string to Field #2457

Conversation

dschwoerer commented Oct 28, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

dschwoerer Oct 28, 2021

Choose a reason for hiding this comment

ZedThree Oct 28, 2021

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

ZedThree commented Oct 28, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

ZedThree Oct 28, 2021

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

ZedThree Oct 28, 2021

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

github-actions bot Oct 28, 2021

Choose a reason for hiding this comment

ZedThree Oct 28, 2021

Choose a reason for hiding this comment

dschwoerer Oct 28, 2021

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 28, 2021

ZedThree commented Oct 28, 2021

ZedThree commented Oct 28, 2021

dschwoerer commented Oct 28, 2021

ZedThree commented Oct 28, 2021