-
Notifications
You must be signed in to change notification settings - Fork 740
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
Expects/Ensures revision #962
Conversation
In microbenchmarks I tried, using `__builtin_expect` to indicate which branch was likely/unlikely was never faster, and often slower, than just omitting. In GSL, this is used only for `Expects` and `Ensures` which are nearly always true, and all processors have long been great at predicting such branches well.
This is not specified in the C++ Core Guidelines as part of GSL, so it should not be documented or supported with a GSL name. And since we don't use it in the GSL repo either, we should just remove it directly. For more about why assumptions are a dangerous blunt instrument, including that they actively eliminate contracts checks including GSL's `Expects` and `Ensures`, see [P2064](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2064r0.pdf).
Allows independently controlling handling of different categories of bugs, such as bounds checks vs. null checks. User-extensible: Companies can instantiate their own `contract_group` objects for their own categories of contract checks, including for distinguishing contract "levels" like `Normal` vs. `Audit` by just creating those two groups that can then be controlled independently or in combination.
Revert changes in multi_span to avoid a merge conflict, since this file was deleted in a parallel PR
Require `chandler` to be never null by installing `[]()noexcept{}` as the handler if given a null pointer. This lets us remove the double test in `assertion`.
…C++17 guaranteed copy elision A concession to older compilers.
As always with feature tests, they can only be used to aggressively adopt new features even before all compilers we need have the features -- the test lets us write a vestigial workaround for downllevel compilers, to be removed as soon as all compilers support the feature.
At least the nonthrowing ones
Accidentally flipped on the previous commit
Removed unnecessary headers Added missing const
} // namespace gsl | ||
|
||
#define Expects(cond, kind) kind.expects(cond) | ||
#define Ensures(cond, kind) kind.ensures(cond) |
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.
Do we want to add an Expects(cond)
and Ensures(cond)
to prevent outright breaking users who have Expects/Ensures in their code or are we willing to make this a breaking change?
And removed the feature test macro to instead tgest for a specific C++ version.
Is this extension of |
Before this PR is accepted, it needs to be reviewed by the Guidelines editors. Summarizing the current status and plan: Right now, we (Microsoft) are first doing a performance test in some internal code that heavily uses GSL (particularly but not only gsl::span) to validate that this proposed change doesn't cause measurable perf overhead. If that efficiency is not validated, there's no point in bothering the Guidelines editors with a non-starter and we should just reject this PR. If it is validated, then the next step is to have the design reviewed by the Guidelines editors and see if they agree, agree with changes or other feedback, or reject it. |
Thank you, that is helpful context. Why do you want to make contract enforcement strictly a runtime decision instead of sticking with the traditional preprocessor-based approach (conditional definition of |
@mbeutel Sorry for the delay in replying. Re this:
We have some customers who want one or both of:
|
@hsutter No problem, thanks for taking the time to explain.
How would that work? How would
(...e.g.
Could you elaborate on that concern? I understand that bifurcation of vocabulary types can be problematic, the prime example being
I agree that it would be desirable to have some granularity in the enforcement of precondition/postcondition checks, and I like the idea of contract groups. Sometimes it's easier to assess how expensive and how useful a check will be than which group it would belong to. Some of the Contracts proposals had floated the idea of audit-level checks, which had inspired me to add different severity-level flavors of
Contract groups could be orthogonal to severity, and I can see how configuring different violation behaviors for different contract groups might be useful. E.g. one might want to terminate on safety-related violations but only emit a log message for violations of program logic. |
This PR does the following:
__builtin_expect
branch prediction assistance, which in strict_not_null performance issues with reference types #930 was measured to be useless or a small pessimization.GSL_ASSUME
which was unused and is not mentioned in the C++ Core Guidelines which defines GSL.GSL_UNENFORCED_ON_CONTRACT_VIOLATION
andGSL_TERMINATE_ON_CONTRACT_VIOLATION
(the latter is still the default)GSL_CONTRACT_CHECK
with a generalcontract_group
, and parameterizesExpects
andEnsures
with a group.The last is the main change. Each
contract_group
object wraps one handler, "terminate_handler" style. Making each one an object allows controlling violation handlers separately for different groups of contracts, notablyBounds
andNull
checks -- now GSL users who wantBounds
violations to always fail-fast can enforce that only those violations do always fail-fast, and independently can configure whetherNull
violations should fail-fast. The default violation action for each group is currently set to beterminate
, but can be configured to do nearly anything by customizingset_handler
-- for example,gsl.Bounds.set_handler( []{ my_custom_logger.LogAndContinue("out of bounds access detected"); } );
.The only restriction on the violation handler is that it must be
noexcept
. This intentionally prevents installing throwing violation handlers, because those are not meaningful in general: Whether a given function wants to writeExpects
/Ensures
contracts for debugging purposes, and/or the same function can never fail and therefore wants to also benoexcept
at run time, are orthogonal and independent issues, and in the case of a function that does both it is not useful to throw an exception from anExpects
/Ensures
violation because it will just hit the function'snoexcept
, so installing a throwing violation handler is just a fancy way of installing a handler that callsterminate
.I've done performance tests at
-O1
and have not been able to measure an overhead to doing thisif (!b) call_handler();
test that is not in the range of instruction alignment issues. That is, in the few tests where I could measure a slight overhead (or speedup!) to inserting theExpects
orEnsures
check, I was able to erase the difference by adding a nop (asm(nop);
), for example here: https://quick-bench.com/q/PqA9EPJqUOwp51iql4NKUDEpsUg . So at least so far it appears there is zero cost to the predictable branch and the only overhead is the extra instructions.More performance testing is welcome -- in particular, @JordanMaples please try this branch vs. the current implementation in any existing
span
performance tests you have and see if there's a measurable overhead onspan
bounds checking now that thespan
bounds check would invoke a user-customizable violation handler on failure.This PR intends to address several open issues: