Reduce Lookups
##Reduce Lookups
ValueT& operator[](KeyT key)
does 2 lookups, one in map.count(key)
and one in map[key]
. Using map.find(key)
and then comparing the iterator against std::end(map)
and then dereferencing it to return the value avoids that.
Consistent Parameter Types
##Consistent Parameter Types
ValueT& operator[](KeyT key)
takes KeyT
by value while other functions use const KeyT &
. There doesn't seem to be a reason for that and you should be consistent.
#Support Move-Only Types
Take
Support Move-Only Types
Take inspiration from the standard. All versions of std::map::insert_or_assign
take a ValueT &&
while you take a const ValueT &
. That means unlike the standard containers you do not support std::unique_ptr
for example. I don't see anything else that is holding it back, so it's an easy improvement for minimal effort.
##Bonus: Use All The New Cool Features
Bonus: Use All The New Cool Features
(this one is not entirely serious)
In insert_or_assign
auto [elem_it, was_new] = ins_res;
if (was_new) order.push_back(&*elem_it);
can be written as
if (auto [elem_it, was_new] = ins_res; was_new) {
order.push_back(&*elem_it);
}
I'm not sure it's better, but it's fancier. In theory it's better because it limits the scope of elem_it
and was_new
, but in practice in this case it just doesn't matter.
##Nitpicking
Some
Nitpicking
Some of your variables can be const
such as ins_res
and map_it
.
##Extensions
You
Extensions
You seem to want to keep it simple, so take these as suggestions of what could be done, not necessarily as part of the code review. ###Transparent Comparators It
Transparent Comparators
It would be cool if you supported lookups that don't require to create a KeyT
. For example ++smap["Johnny"];
unnecessarily creates a temporary std::string
. std::string
can compare to const char *
already. See transparent comparators and std::map::find
for inspiration.
###A Real Container
Maybe
A Real Container
Maybe you could make SequencedMap
a real container which then allows its use in all the standard algorithms.
##When In Doubt Use std::vector
It's
When In Doubt Use std::vector
It's a good default container unless benchmarks show you need something else. std::list
is legendary for its terrible performance in almost all circumstances, even the ones that sound like they should be faster such as removing an element from the middle.
##Reduce Lookups
ValueT& operator[](KeyT key)
does 2 lookups, one in map.count(key)
and one in map[key]
. Using map.find(key)
and then comparing the iterator against std::end(map)
and then dereferencing it to return the value avoids that.
##Consistent Parameter Types
ValueT& operator[](KeyT key)
takes KeyT
by value while other functions use const KeyT &
. There doesn't seem to be a reason for that and you should be consistent.
#Support Move-Only Types
Take inspiration from the standard. All versions of std::map::insert_or_assign
take a ValueT &&
while you take a const ValueT &
. That means unlike the standard containers you do not support std::unique_ptr
for example. I don't see anything else that is holding it back, so it's an easy improvement for minimal effort.
##Bonus: Use All The New Cool Features
(this one is not entirely serious)
In insert_or_assign
auto [elem_it, was_new] = ins_res;
if (was_new) order.push_back(&*elem_it);
can be written as
if (auto [elem_it, was_new] = ins_res; was_new) {
order.push_back(&*elem_it);
}
I'm not sure it's better, but it's fancier. In theory it's better because it limits the scope of elem_it
and was_new
, but in practice in this case it just doesn't matter.
##Nitpicking
Some of your variables can be const
such as ins_res
and map_it
.
##Extensions
You seem to want to keep it simple, so take these as suggestions of what could be done, not necessarily as part of the code review.
###Transparent Comparators
It would be cool if you supported lookups that don't require to create a KeyT
. For example ++smap["Johnny"];
unnecessarily creates a temporary std::string
. std::string
can compare to const char *
already. See transparent comparators and std::map::find
for inspiration.
###A Real Container
Maybe you could make SequencedMap
a real container which then allows its use in all the standard algorithms.
##When In Doubt Use std::vector
It's a good default container unless benchmarks show you need something else. std::list
is legendary for its terrible performance in almost all circumstances, even the ones that sound like they should be faster such as removing an element from the middle.
Reduce Lookups
ValueT& operator[](KeyT key)
does 2 lookups, one in map.count(key)
and one in map[key]
. Using map.find(key)
and then comparing the iterator against std::end(map)
and then dereferencing it to return the value avoids that.
Consistent Parameter Types
ValueT& operator[](KeyT key)
takes KeyT
by value while other functions use const KeyT &
. There doesn't seem to be a reason for that and you should be consistent.
Support Move-Only Types
Take inspiration from the standard. All versions of std::map::insert_or_assign
take a ValueT &&
while you take a const ValueT &
. That means unlike the standard containers you do not support std::unique_ptr
for example. I don't see anything else that is holding it back, so it's an easy improvement for minimal effort.
Bonus: Use All The New Cool Features
(this one is not entirely serious)
In insert_or_assign
auto [elem_it, was_new] = ins_res;
if (was_new) order.push_back(&*elem_it);
can be written as
if (auto [elem_it, was_new] = ins_res; was_new) {
order.push_back(&*elem_it);
}
I'm not sure it's better, but it's fancier. In theory it's better because it limits the scope of elem_it
and was_new
, but in practice in this case it just doesn't matter.
Nitpicking
Some of your variables can be const
such as ins_res
and map_it
.
Extensions
You seem to want to keep it simple, so take these as suggestions of what could be done, not necessarily as part of the code review.
Transparent Comparators
It would be cool if you supported lookups that don't require to create a KeyT
. For example ++smap["Johnny"];
unnecessarily creates a temporary std::string
. std::string
can compare to const char *
already. See transparent comparators and std::map::find
for inspiration.
A Real Container
Maybe you could make SequencedMap
a real container which then allows its use in all the standard algorithms.
When In Doubt Use std::vector
It's a good default container unless benchmarks show you need something else. std::list
is legendary for its terrible performance in almost all circumstances, even the ones that sound like they should be faster such as removing an element from the middle.
##Reduce Lookups
ValueT& operator[](KeyT key)
does 2 lookups, one in map.count(key)
and one in map[key]
. Using map.find(key)
and then comparing the iterator against std::end(map)
and then dereferencing it to return the value avoids that.
##Consistent Parameter Types
ValueT& operator[](KeyT key)
takes KeyT
by value while other functions use const KeyT &
. There doesn't seem to be a reason for that and you should be consistent.
#Support Move-Only Types
Take inspiration from the standard. All versions of std::map::insert_or_assign
take a ValueT &&
while you take a const ValueT &
. That means unlike the standard containers you do not support std::unique_ptr
for example. I don't see anything else that is holding it back, so it's an easy improvement for minimal effort.
##Bonus: Use All The New Cool Features
(this one is not entirely serious)
In insert_or_assign
auto [elem_it, was_new] = ins_res;
if (was_new) order.push_back(&*elem_it);
can be written as
if (auto [elem_it, was_new] = ins_res; was_new) {
order.push_back(&*elem_it);
}
I'm not sure it's better, but it's fancier. In theory it's better because it limits the scope of elem_it
and was_new
, but in practice in this case it just doesn't matter.
##Nitpicking
Some of your variables can be const
such as ins_res
and map_it
.
##Extensions
You seem to want to keep it simple, so take these as suggestions of what could be done, not necessarily as part of the code review.
###Transparent Comparators
It would be cool if you supported lookups that don't require to create a KeyT
. For example ++smap["Johnny"];
unnecessarily creates a temporary std::string
. std::string
can compare to const char *
already. See transparent comparators and std::map::find
for inspiration.
###A Real Container
Maybe you could make SequencedMap
a real container which then allows its use in all the standard algorithms.
##When In Doubt Use std::vector
It's a good default container unless benchmarks show you need something else. std::list
is legendary for its terrible performance in almost all circumstances, even the ones that sound like they should be faster such as removing an element from the middle.