[SERVER-54836] tassert() should use operator bool() Created: 26/Feb/21 Updated: 29/Oct/23 Resolved: 10/Mar/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | None |
| Fix Version/s: | 4.9.0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | David Storch | Assignee: | Yoon Soo Kim |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | neweng | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Sprint: | Query Execution 2021-03-22 | ||||||||
| Participants: | |||||||||
| Description |
|
Let's say I have a boost::optional<T>. I would like to be able to write the following:
However, today I have to write either
or
|
| Comments |
| Comment by Githook User [ 10/Mar/21 ] | ||
|
Author: {'name': 'Yoonsoo Kim', 'email': 'yoonsoo.kim@mongodb.com', 'username': 'yun-soo'}Message: | ||
| Comment by Billy Donahue [ 06/Mar/21 ] | ||
|
I would have some feedback on the details if we went to code review, but yes the preprocessor arity-dispatch is the right direction. Yes we do have to support MSVC. There are workarounds for MSVC. See boost/preprocessor and util/invariant.h. I am working on this problem right now, under There are a few other related points I want to hit while I'm in this file though, so I want to avoid a situation where we have competing changes. | ||
| Comment by Billy Donahue [ 06/Mar/21 ] | ||
|
Changes to util/assert_util.h should involve Service Architecture. If it breaks, that's who would be on the hook to fix it. To address the original description's problem, I'd suggest a third option. The shortest way to get an explicit bool context is with !!, so:
But I agree that this behavior of tassert doesn't meet user expectations. This is one of several tassert implementation flaws and we should handle them holistically and with careful design. The origin of this problem is that the tassert macro doesn't expand the condition within an internal if statement as a caller would expect, and the way every other assert macro does. This is perhaps because it was written as an outsider contribution, I don't know. I know it doesn't match the implementation style of the other asserts and there's no technical need for that, as far as I can tell. This has other knock-on effects, like forcing the construction of a SourceLocation and std::string msg (EXPENSIVE!) even in the success case where those objects won't be used. iassert has exactly the same implementation flaws and must be fixed in the same way, preferably at the same time. I think it's simpler to fix all of this at once, as patching in a small way to do JUST an explicit bool cast will not fix those other problems and can make them a little more annoying to track and backport the effort. I'm marking this ticket as "caused by" |