Default messages in assertion macros #12019
Merged
Conversation
This file contains 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
|
Looks like MSVC does not like variadic macros very much... |
cameel
commented
Sep 23, 2021
Comment on lines
+74
to
+80
| /// Assertion that throws an exception containing the given description if it is not met. | ||
| /// Use it as assertThrow(1 == 1, ExceptionType, "Mathematics is wrong."); | ||
| /// The second parameter must be an exception class (rather than an instance). | ||
| #define assertThrow(_condition, _exceptionType, _description) \ | ||
| assertThrowWithDefaultDescription(_condition, _exceptionType, _description, "Assertion failed") |
There was a problem hiding this comment.
Is assertThrow() really an assertion? In most cases I've seen it's just used as a convenience helper to throw a boost exception.
cameel
force-pushed
the
default-messages-in-assert-macros
branch
from
ee2ae56 to
07d53b5
Compare
Looks like it requires extra |
cameel
force-pushed
the
default-messages-in-assert-macros
branch
from
07d53b5 to
87a2245
Compare
|
By the way, is this something that should have a changelog entry? This might affect tools that try to detect some specific ICEs in the output and do some workarounds. We might want to at least inform them even if we do not guarantee any backwards-compatibility for these messages. |
cameel
force-pushed
the
default-messages-in-assert-macros
branch
from
87a2245 to
83ba149
Compare
|
I don't think we should guarantee backwards compatibility in ICEs |
leonardoalt
reviewed
Oct 1, 2021
| @@ -1217,7 +1217,7 @@ string YulUtilFunctions::extractByteArrayLengthFunction() | |||
| std::string YulUtilFunctions::resizeArrayFunction(ArrayType const& _type) | |||
| { | |||
| solAssert(_type.location() == DataLocation::Storage, ""); | |||
| solUnimplementedAssert(_type.baseType()->storageBytes() <= 32, "..."); | |||
leonardoalt
reviewed
Oct 1, 2021
cameel
force-pushed
the
default-messages-in-assert-macros
branch
from
83ba149 to
514b9a0
Compare
leonardoalt
previously approved these changes
Oct 1, 2021
|
Needs rebase |
chriseth
force-pushed
the
default-messages-in-assert-macros
branch
from
514b9a0 to
7f71074
Compare
chriseth
approved these changes
Oct 4, 2021
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.
This is an attempt to improve the situation in #12017 a little. It should also be an improvement for external tools because for these our ICEs are often puzzling when they have no message at all (see e.g. trufflesuite/truffle#2849).
""in an assertion, it gets replaced with a generic message, like"Solidity assertion failed"or"Unimplemented feature", depending on the assertion type.solAssert()and other assertions, making them more convenient.