-
Notifications
You must be signed in to change notification settings - Fork 274
Exception utils: remove redundant what() implementations #6608
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
Exception utils: remove redundant what() implementations #6608
Conversation
The change means that there are two mechanisms for communicating the |
Codecov Report
@@ Coverage Diff @@
## develop #6608 +/- ##
========================================
Coverage 76.73% 76.73%
========================================
Files 1579 1579
Lines 181999 181994 -5
========================================
- Hits 139652 139650 -2
+ Misses 42347 42344 -3
Continue to review full report at Codecov.
|
Isn't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one comment.
@@ -23,7 +21,7 @@ class smt2_tokenizert | |||
line_no=1; | |||
} | |||
|
|||
class smt2_errort : public cprover_exception_baset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this was removed from the inheritance chain? Would it be better if message
was removed and substituted with reason
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this class, message
is a std::ostringstream
, making it really different from the parent class. Really, there is no meaningful commonality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only benefit would be to catch all these exceptions at once, which could be done by another 'base class with reason' above the existing base, but that's minor IMO.
824db62
to
a3cca59
Compare
Make a `std::string reason` a member of the base class, and make `what()` default to returning `reason`. This avoids duplicating the same implementation across several child classes. While at it, also fix the missing init-to-nil in one of incorrect_goto_program_exceptiont's constructors.
a3cca59
to
86b13e9
Compare
Make a
std::string reason
a member of the base class, and makewhat()
default to returningreason
. This avoids duplicating the sameimplementation across several child classes.
While at it, also fix the missing init-to-nil in one of
incorrect_goto_program_exceptiont's constructors.