Skip to content

Conversation

@varunagrawal
Copy link
Contributor

@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
Contributor 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
Contributor 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
Contributor 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
Contributor 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
Contributor 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.

3 participants