[SERVER-42081] Map helpers for strict use Created: 03/Jul/19  Updated: 10/Sep/19  Resolved: 10/Sep/19

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Benjamin Caimano (Inactive) Assignee: Billy Donahue
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Sprint: Dev Tools 2019-09-09, Dev Tools 2019-09-09, Dev Tools 2019-09-23
Participants:

 Description   

I recently ended up creating translation unit-static functions for more strict access patterns with stdx::unordered_map/_set. I suspect that a set of helpers with invariants might be more useful to the codebase at large:

  • getOrInvariant() which invariants that the key already exists (roughly equivalent to at()).
  • emplaceOrInvariant() which invariants that the key did not already exist.
  • eraseOrInvariant() which invariants that the key existed and will no longer.

This set of operations allows a developer to confidently use a map or set to contain a unique set of initialized objects.



 Comments   
Comment by Billy Donahue [ 10/Sep/19 ]

My feeling on functions like this is that you're better off writing the code out where it's needed.

I overhauled Google's util/gtl "helpers" collection and it was like this huge pile of fiddly little functions all with slightly different behaviors. In the end, C+11, particularly `unique_ptr`, `auto`, and `rvalue references`, left most of the helpers as being far more trouble than they were worth. They had caveats that caused a few bugs (like FindWithDefault's dangling reference on the `default` argument), and developers relied on them too much, and didn't develop their skills working with C+ containers as well as they should have.

There are issues here like:

What should be the exact signature of getOrInvariant? Is the key a `const Map::key_type&`? That ignores heterogeneous lookup, or hinted find. Pretty soon they might want countOrInvariant, equalRangeOrInvariant....

emplaceOrInvariant has more problems, the argument list is easier (M&, const_iterator, Args&&...). Does it take a hint? But then will we soon want emplaceHintOrInvariant(M&, K&, Args&&...), tryEmplaceOrInvariant(M&, const_iterator, Args&&...), insertOrInvariant(M&, const value_type&), and insertOrInvariant(M&, value_type&&) ? What happens if an exception is thrown while emplacing? Is that allowed to propagate out or is Invariant the only possibility if the emplace doesn't succeed. It gets to be a mouthful.

When I'm reading code and encounter helper function, I have to think about the signature, return type, and possible caveats of the helper function. Whereas if I read straight container-manipulating code I just have to know the container API and a few idioms. In practice I think the manipulator functions are only saving about 2 or 3 lines of code, and you can't really be sure they were the only functions used to manipulate an arbitrary container. It will have back doors unless you encapsulate it and make the invariant enforcement part of the capsule, so then the capsule is the thing we'd need to design, and IMO it's very hard to do it in a way that anticipates the diversity in user requirements.

Like, what's so special about `invariant`? Why isn't the "or" action customizable to other things? Okay how do we specify those other actions? You start to get into this maze of twisty little passages.

I don't want to be down on this idea, but I have seen a very similar collection of algorithms at Google, and worked on an assignment for several months on paring it down (some things had 50k+ callers) and getting rid of it, and I just don't think anyone who looked at the problems up close was really convinced that the convenience of those helpers was worthwhile post-C++11.

When I have a special map that needs special restrictions, I like to make a little class or derived type containing that map, and have it expose only those operations I want to allow. But making it a public type in a util/ directory is tough because you can't really account for all the variations the users will want and perhaps even expect it to have. My feeling is that this ticket should probably be closed.

Comment by Benjamin Caimano (Inactive) [ 23/Jul/19 ]

Yeah, that would be fine by me. I've already caught one runtime bug with these helpers in the last week.

Comment by Billy Donahue [ 22/Jul/19 ]

Maybe you need a facade type around the map to enforce this discipline for
you rather than a collection of functions?

On Mon, Jul 22, 2019 at 2:42 PM Andrew Morrow (Jira) <jira@mongodb.org>

Generated at Thu Feb 08 04:59:32 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.