Add string2optional conversion functions #4192

hannes-steffenhagen-diffblue · 2019-02-14T20:12:36Z

These are intended as helpers for when you want to convert a string to
and integer, but don't want to assert the result (e.g. when dealing with
user input there's usually something better you can do than outright
crashing if the conversion fails), but also don't want to deal with
exceptions (which involve more code to write and read and it's easy to
handle the wrong set of exceptions, whether it is too many or too few).

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

xbauch

Looks good.

tautschnig

Ok, but could we also replace the existing str2number code by the new implementation to avoid duplication? str2number would then possibly assert when getting nullopt.

tautschnig · 2019-02-14T20:38:03Z

unit/Makefile

@@ -70,6 +70,7 @@ SRC += analyses/ai/ai.cpp \
       util/small_map.cpp \
       util/small_shared_two_way_ptr.cpp \
 	util/std_expr.cpp \
+	   util/string2int.cpp \


The indents here look pretty weird (likely tab/space mix) - both the newly added line as well as the one above.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: a7c0f48).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/100997735

romainbrenguier · 2019-02-15T09:43:41Z

src/util/string2int.cpp

+  }
+}
+
+optionalt<int> string2optional_int(const std::string &str, int base)


These should be templates, as is done for numeric_cast.

Ok, I'll write one

romainbrenguier · 2019-02-15T09:45:50Z

src/util/string2int.cpp

+    }
+    else
+    {
+      throw std::out_of_range{"conversion out of range"};


Why throw and then wrap the function in a try catch to return an optional? Return an optional directly.

Because sto* type functions throw on failure. The function that's passed into wrap is basically just sto, but I need to do some additional checks on the string before passing it to stol as well as checking the conversion result and it makes sense to just use the same interface for that.

I would prefer wrapping just the std::stoul in that case. The string in the exception here is never used and so is just a comment. That's misleading because we would expect it to be useful somewhere.

romainbrenguier · 2019-02-15T11:13:05Z

src/util/string2int.cpp

+    }
+    auto const ul_value = std::stoul(str, nullptr, base);
+    auto const u_value = narrow_cast<unsigned>(ul_value);
+    if(u_value == ul_value)


This is not pretty. What's the difference with just returning the result of numeric_cast<unsigned> ?

EDIT my previous comment was based on a misreading of your comment, I'll double check

Sorry I thought we had implemented a numeric_cast from integral type to integral type but apparently it's just from mp_integer (or exprt) to integral type.

Ideally, it would be nice to extend numeric_cast and use it here, but it doesn't need to be in this PR.
You could write if(ul_value <= std::numeric_limits<T>::max() && ul_value >= std::numeric_limits<T>::min()) before doing the conversion (I don't know if that's much better).

hannes-steffenhagen-diffblue · 2019-02-15T11:31:20Z

@romainbrenguier @tautschnig

There is now a template string2optional which can be used to convert to any of the standard integer types as long as they're smaller than (unsigned) long long. The indentation in the unit Makefile has been fixed. The safe_* functions now defer back to the optional versions, the unsafe versions use the strto* family directly now.

It's a bit concerning that we even need the unsafe versions, there are several tests that are failing if we don't leave the conversions unchecked.

NlightNFotis

Solid. Thanks.

tautschnig · 2019-02-15T11:38:28Z

It's a bit concerning that we even need the unsafe versions, there are several tests that are failing if we don't leave the conversions unchecked.

Yes, it would be very nice to get rid of all the unsafe_* ones, but that's presumably a lot more work. For now, at least it's easy to git grep for them...

These are intended as helpers for when you want to convert a string to and integer, but don't want to assert the result (e.g. when dealing with user input there's usually something better you can do than outright crashing if the conversion fails), but also don't want to deal with exceptions (which involve more code to write and read and it's easy to handle the wrong set of exceptions, whether it is too many or too few).

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: e9ccc27).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/101085557

hannes-steffenhagen-diffblue requested review from kroening, peterschrammel, pkesseli, smowton and tautschnig as code owners February 14, 2019 20:12

hannes-steffenhagen-diffblue force-pushed the string_to_optional branch from ff21b7d to a7c0f48 Compare February 14, 2019 20:18

hannes-steffenhagen-diffblue mentioned this pull request Feb 14, 2019

Non-deterministic initialisation of structs, pointers and globals in function call harness #4181

Merged

7 tasks

xbauch approved these changes Feb 14, 2019

View reviewed changes

tautschnig approved these changes Feb 14, 2019

View reviewed changes

tautschnig assigned hannes-steffenhagen-diffblue Feb 14, 2019

allredj reviewed Feb 14, 2019

View reviewed changes

chrisr-diffblue approved these changes Feb 15, 2019

View reviewed changes

romainbrenguier reviewed Feb 15, 2019

View reviewed changes

danpoe approved these changes Feb 15, 2019

View reviewed changes

hannes-steffenhagen-diffblue force-pushed the string_to_optional branch from a7c0f48 to d594ab3 Compare February 15, 2019 11:34

NlightNFotis approved these changes Feb 15, 2019

View reviewed changes

hannes-steffenhagen-diffblue force-pushed the string_to_optional branch from d594ab3 to e9ccc27 Compare February 15, 2019 12:24

allredj reviewed Feb 15, 2019

View reviewed changes

hannes-steffenhagen-diffblue merged commit a62b5da into diffblue:develop Feb 15, 2019

Add string2optional conversion functions #4192

Add string2optional conversion functions #4192

Uh oh!

Conversation

hannes-steffenhagen-diffblue commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xbauch left a comment

Choose a reason for hiding this comment

Uh oh!

tautschnig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannes-steffenhagen-diffblue Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannes-steffenhagen-diffblue commented Feb 15, 2019

Uh oh!

NlightNFotis left a comment

Choose a reason for hiding this comment

Uh oh!

tautschnig commented Feb 15, 2019

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hannes-steffenhagen-diffblue commented Feb 14, 2019 •

edited

Loading

hannes-steffenhagen-diffblue Feb 15, 2019 •

edited

Loading