Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

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.

Source Link
nwp
  • 1.6k
  • 11
  • 14

##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.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /