[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:
Problem/Incident
is caused by SERVER-55017 iassert/tassert evaluate arguments wh... Closed
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:

boost::optional<T> myT = getT();
tassert(1234500, "expected a non-none T", myT);

However, today I have to write either

tassert(1234500, "expected a non-none T", static_cast<bool>(myT));

or

tassert(1234500, "expected a non-none T", myT.has_value());



 Comments   
Comment by Githook User [ 10/Mar/21 ]

Author:

{'name': 'Yoonsoo Kim', 'email': 'yoonsoo.kim@mongodb.com', 'username': 'yun-soo'}

Message: SERVER-54836 Cleaned up a few of static_cast<bool> and has_value() calls
Branch: master
https://github.com/mongodb/mongo/commit/4c74e17d5c2166e1347244652bd3ba7d50d034ef

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 SERVER-55017.

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:

tassert(1234500, "expected a non-none T", myT);  // error
tassert(1234500, "expected a non-none T", !!myT);  // fine

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" SERVER-55017, which is a ticket I hope to expedite and which should address the larger issue common to iassert and tassert.

Generated at Thu Feb 08 05:34:38 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.