[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:
Related
related to SERVER-65278 fix clang-tidy v4 issues and add v4 c... Closed
is related to SERVER-58110 IDL: getters sometimes have const val... Closed
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/
(Thx redbeard0531 for the link)



 Comments   
Comment by Billy Donahue [ 25/Apr/22 ]

This simplification makes SERVER-58110 easier to do

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:

  • object_owned -> object
  • object -> object_unowned

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.

cc andrew.shuvalov

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