[SERVER-55449] Add a tassertStatusOK() macro to assert_util.h Created: 23/Mar/21 Updated: 06/Dec/22 Resolved: 26/Mar/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Minor - P4 |
| Reporter: | Timour Katchaounov | Assignee: | Backlog - Service Architecture |
| Resolution: | Duplicate | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Assigned Teams: |
Service Arch
|
||||||||||||
| Participants: | |||||||||||||
| Description |
|
A common code pattern is to ensure that the result of some operation was Status::OK(). Example:
|
| Comments |
| Comment by Githook User [ 12/May/21 ] |
|
Author: {'name': 'Amirsaman Memaripour', 'email': 'amirsaman.memaripour@mongodb.com', 'username': 'samanca'}Message: |
| Comment by Billy Donahue [ 26/Mar/21 ] |
|
Maybe trying to give tassert/iassert a return value similar to that of uassertStatusOK(StatusWith<T>) should be considered as part of SERVER-53380. |
| Comment by Billy Donahue [ 26/Mar/21 ] |
|
I think we'll just close it. The work to give uniform XassertX API support is really SERVER-53380. |
| Comment by David Storch [ 26/Mar/21 ] |
|
Oh! Whoops! Yeah, the original request was not about extracting a T from a StatusWith<T>. I did not realize we already supported tassert(status), which is the same as tassertStatusOK() but by a different name. In that case I think this can just be closed, unless you want to convert the ticket into the auto val = tassert(sw); request. |
| Comment by Billy Donahue [ 26/Mar/21 ] |
|
david.storch, would the proposed tassertStatusOK(status) be different from the existing tassert(status) ? Maybe the extraction of value from a StatusWith<T> in a auto val = tassert(sw); would be nice. But it wasn't explicitly mentioned in the ticket I don't think, so I want to make sure I understand exactly what the request is. That is, I think we can already do what's asked for, it just doesn't have the -StatusOK suffix on its name. Thx. |
| Comment by David Storch [ 26/Mar/21 ] |
|
Ah, good points Billy. I think the feature request is slightly misstated in the description. It would be about adding a tassertStatusOK() that works exactly like invariantStatusOK(). It would take just a Status, not a Status, code, and reason. I'll clarify the description. I do not believe the intent was to propose a different API for tassert() compared to the other assertion macros. I will add this to the triage queue for the Service Arch team. |
| Comment by Billy Donahue [ 24/Mar/21 ] |
|
Yes. I mean we should discuss it in a triage meeting as a feature request. You can already tassert(Status) or iassert(Status) directly. Why does tassertStatusOK take an error code and a reason when the status already has these attributes? Is `tassertStatusOK` different from `invariantStatusOK`, `uassertStatusOK`, or `massertStatusOK`? If it needs a msgid, then it's more analogous to fassertFailedWithStatus(msgid,status), but tassert doesn't otherwise follow fassert's API so maybe that would need to change. We'll try to make it so that all the XassertX macros should have the same API if possible. Maybe even the same shared implementation (SERVER-53380). So if tassertStatusOK takes a code and reason, then ideally so would all the other XStatusOK macros. So this ticket is one of:
The big picture is that we should be trying to make all the asserts have consistent feature set and calling convention. Right now they're very difficult to reason about, and the proposed tassert(msgid,msg,status) looks like it would be yet another syntax unique to tassert, which would make the XassertX API consistency problem a little worse. |
| Comment by David Storch [ 24/Mar/21 ] |
|
billy.donahue is this something that the Service Arch team would consider picking up? |