Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new insert_or_assign method to Values #959

Merged
merged 2 commits into from
Dec 16, 2021
Merged

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Dec 10, 2021

This PR adds a new method insert_or_assign in support of #958.

Modern databases support the upsert method which tries to perform an update and, if a key doesn't exist, defaults to an insert.

insert_or_assign is a C++17 std::map method so this method aligns us better with that.

This simplifies this common pattern:

if (values.exists(key)) {
	values.update(key, value);
} else {
	values.insert(key, value);
}

to the more concise:

values.insert_or_assign(key, value);

@varunagrawal varunagrawal added the feature New proposed feature label Dec 10, 2021
@varunagrawal varunagrawal self-assigned this Dec 10, 2021
@varunagrawal varunagrawal added the quick-review Quick and easy PR to review label Dec 10, 2021
@dellaert
Copy link
Member

Already exists as tryInsert ?

@varunagrawal
Copy link
Collaborator Author

tryInsert doesn't perform an update if the key already exists. upsert will perform the update.

@dellaert
Copy link
Member

I’m not in love with this. If we modify Values, I’d prefer to align it more with stl::map, especially the c++11 and c++17 versions.

It’s probably better to do a design-review these PRs to the core data structures via an issue that proposes a change, before spending a lot of time.

If you disagree strongly, let’s discuss off-line.

@varunagrawal
Copy link
Collaborator Author

varunagrawal commented Dec 13, 2021

Adding some research I did here on templated operator overloading, thanks to this link.
Apparently it is possible. We can define this:

template <typename T>
T& operator[](Key k) {
...
}

but the operator call would be ugly

Values values;
Pose3 pose;
values[key] = pose; // Compiler error due to template mismatch!!
values.operator[]<Pose3>(pose);  // Works, but is more verbose and defeats the purpose.

@dellaert
Copy link
Member

Yeah, that’s really ugly. Did you read the entire c++17 interface to standard map though? Is there another function that already exists that does what the imperative operator does?

@varunagrawal
Copy link
Collaborator Author

I did. There's insert_or_assign which is exactly what upsert is. Should we rename upsert to that?

I guess the cpp standards committee likes verbose function names.

@dellaert
Copy link
Member

Yeah. I like shorter names as well, but insert_or_assign will create less surprise for developers.

@varunagrawal
Copy link
Collaborator Author

Cool, I'll update the method name. :)

@varunagrawal varunagrawal changed the title Add new upsert method to Values Add new insert_or_assign method to Values Dec 15, 2021
@varunagrawal
Copy link
Collaborator Author

@dellaert updated

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice !

@varunagrawal varunagrawal merged commit e18ecc3 into develop Dec 16, 2021
@varunagrawal varunagrawal deleted the values/upsert branch December 16, 2021 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New proposed feature quick-review Quick and easy PR to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants