Refactoring of the parsing of the --object-bits command line argument
#5447
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thomasspriggs
requested review from
kroening,
peterschrammel,
pkesseli,
smowton and
tautschnig
as code owners
hannes-steffenhagen-diffblue
approved these changes
Aug 11, 2020
src/util/config.cpp
Outdated
| @@ -776,8 +776,8 @@ configt::bv_encodingt parse_object_bits_encoding( | |||
| const std::string &argument, | |||
| const std::size_t pointer_width) | |||
| { | |||
| const unsigned int object_bits = unsafe_string2unsigned(argument); | |||
| if(object_bits <= 0 || object_bits >= pointer_width) | |||
There was a problem hiding this comment.
⛏️ Would be better if the case where parsing fails and the object_bits are out of range were separated, also it would be helpful for debugging to put argument in the error message (this makes it easier to find if for example you've messed up a command line when copy&pasting, e.g. --object-bits --function hello world 8)
hannes-steffenhagen-diffblue
approved these changes
Aug 11, 2020
smowton
approved these changes
Aug 11, 2020
| ^EXIT=1$ | ||
| ^SIGNAL=0$ | ||
| -- | ||
| Test parsing of an invalid non numeric object-bits setting. |
There was a problem hiding this comment.
This is a must-not-match, not a comment (similarly others)
src/util/config.cpp
Outdated
| const std::size_t pointer_width) | ||
| { | ||
| const auto object_bits = string2optional<unsigned int>(argument); | ||
| if(!object_bits || *object_bits <= 0 || *object_bits >= pointer_width) |
There was a problem hiding this comment.
Note <= 0 on an unsigned type
There was a problem hiding this comment.
I have swapped this to use == 0, as this should be less misleading.
In order to ensure that this functionality is not broken by subsequent refactorings.
So that `bv_encodingt` can be more used without the need to explicitly initialise its members.
So the length of `configt::set` can be marginally reduced. So that this functionality can be documented. This would also support unit testing of this parsing if unit tests are added later.
So that direct usage of this function can be more succinct.
The previous use of `unsafe_string2unsigned` handled non numeric input based on the behaviour of 0 being returned by `unsafe_string2unsigned`, which is then caught by the subsequent range check. It was well defined behaviour of `std::strtoul`. However this was only discoverable, based on looking at the implementation of `unsafe_string2unsigned` and reading the documentation of `std::strtoul` or by using a debugger. The explict check in the refactored version makes the handling of this case clear from reading `parse_object_bits_encoding` only, without reference to other code or documentation.
It is easier to debug your invocation of a command line tool if the specific invalid argument and the reason why it is invalid is given.
`object_bits <= 0` suggests that `object_bits` could be below zero. This is not possible because `object_bits` is unsigned. Checking for equal to zero makes it explicit that we are checking for one specific value.
thomasspriggs
force-pushed
the
tas/object_bits_parsing_refactor
branch
from
dd4dac9 to
8efa7cc
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5447 +/- ##
========================================
Coverage 68.22% 68.22%
========================================
Files 1178 1178
Lines 97580 97579 -1
========================================
+ Hits 66573 66575 +2
+ Misses 31007 31004 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
chrisr-diffblue
approved these changes
Aug 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As I was working on #5440, I found the entry point code less than straight forward to follow. I carried out this refactorings locally in order to make it easier for me to understand. I am raising this separate PR containing the refactoring work only as it was not required in order to implement the feature in the original PR, but should still improve maintainability.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/My commit message includes data points confirming performance improvements (if claimed).