[SERVER-58111] IDL: remove ref-qualification on getters Created: 28/Jun/21 Updated: 29/Oct/23 Resolved: 29/Apr/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 6.1.0-rc0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Billy Donahue | Assignee: | Billy Donahue |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | sa-remove-fv-backlog-22 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||
| Sprint: | Service Arch 2022-05-02, Service Arch 2022-05-16 | ||||||||||||
| Participants: | |||||||||||||
| Description |
|
This is an attempt to guard against calling getters on temporaries, a harmless thing to do that can't be prevented by this technique anyway. Getting rid of this stuff will simplify the IDL generator and give us generated classes with behavior that is less surprising. https://quuxplusone.github.io/blog/2019/03/11/value-category-is-not-lifetime/ |
| Comments |
| Comment by Billy Donahue [ 25/Apr/22 ] |
|
This simplification makes |
| Comment by Andrew Shuvalov (Inactive) [ 06/Jul/21 ] |
|
Do I understand correctly that this non-ownership confusion happens with bulk file reads but not with Network consumption where we read a message at a time? I am not sure it's a good idea but we can thing about multiple BSONobjs owning a single large buffer. I am not sure that we can expect that objects bulk read from the file usually have similar lifetime, so if they collectively own a single large buffer via refcount, they will release it at once. This is really just to save one extra mem copy. |
| Comment by Daniel Gottlieb (Inactive) [ 06/Jul/21 ] |
|
While not exactly a reference type, our default IDL code generation for BSONObj ("type: object") has the analogous problem (which is arguably more pervasive and harder to diagnose). The IDL parser does not create a copy of its underlying bytes and rather relies on the underlying bytes outliving the parsed value. This has resulted in annoying to debug errors, particularly for those not indoctrinated in our BSONObj ownership contract. We have since come up with "object_owned" as a safer alternative for the (IMO) more general case. I'd like to see a change where we rename:
Admittedly, this is a more pervasive change and we'd need to re-educate those in the know. If there's agreement from SA that this would be an improvement, but under a different ticket, I'd be happy to file a separate one. |