assertions: add new class of known issue assertions#9814
assertions: add new class of known issue assertions#9814goaway wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Mike Schore <mike.schore@gmail.com>
mattklein123
left a comment
There was a problem hiding this comment.
At a high level I think this is a reasonable way of dealing with the issue you are facing, but needs fleshing out. Also, can you actually use it where you intend to use it so we can see what it looks like? Thank you.
/wait
| // This non-implementation ensures that its argument is a valid expression that can be statically | ||
| // casted to a bool, but the expression is never evaluated and will be compiled away. |
There was a problem hiding this comment.
Can you dedup the body of this with the other place(s) this appears?
| ) | ||
|
|
||
| config_setting( | ||
| name = "debug_known_issues", |
There was a problem hiding this comment.
Can this be a bit more clear on naming? "ignore_known_issues_asserts" or something like that? And just make it a boolean? It's not clear what this does from a quick read. This also needs documenting in https://github.com/envoyproxy/envoy/tree/master/bazel#enabling-optional-features
| * should always pass but that sometimes fails for an unknown reason. The macro allows it to | ||
| * be temporarily compiled out while the failure is triaged and investigated. | ||
| */ | ||
| #define KNOWN_ISSUE_ASSERT(X, DETAILS) _ASSERT_IMPL(X, #X, abort() DETAILS) |
There was a problem hiding this comment.
Do you need release and non-release variants of this? Also can't you just define in terms of ASSERT and RELEASE_ASSERT?
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Description: Adds a new class of assertions, KNOWN_ISSUE_ASSERT, that may be compiled out even when other assertions are compiled in. The intent is to allow assertions to generally be compiled into releases, but to exclude instances that are known to fail for as-yet unknown or unresolved reasons. When the issue has been triaged and resolved, a KNOWN_ISSUE_ASSERT should generally become a regular ASSERT or RELEASE_ASSERT again.
Risk Level: Moderate
Testing: Not yet
Signed-off-by: Mike Schore mike.schore@gmail.com