Skip to content

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented May 3, 2024

Also

  • move-construct underlying iterators in the constructors of flat_(multi)map's iterators;
  • fix and test flat_(multi)map's comparison;
  • remove possibly pessimizing reserve, with signedness adjusted; and
  • simply vector<T, allocator<T>> to vector<T>.

Address review comments in #4468; towards #2910.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner May 3, 2024 08:09
Comment on lines +783 to +784
return _RANGES equal(_Left_base._Data.keys, _Right_base._Data.keys)
&& _RANGES equal(_Left_base._Data.values, _Right_base._Data.values);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we sometimes call _RANGES equal(_Left_base, _Right_base), especially when vectorization is unavailable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that even if the comparison is not vectorized, it may be still advantageous to avoid jumping between arrays, and have dedicated cache/prefetch use just for one array. Though this advantage looks not very significant though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more wondering how this can work for multimap with equivalent keys with distinct values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm more wondering how this can work for multimap with equivalent keys with distinct values.

In flat_multimap, multiple equivalent keys are stored multiple times and each instance is grouped with it's corresponding value, so ranges::equal just works.

Copy link
Contributor

Choose a reason for hiding this comment

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

But is order of values defined?
Like {4, 10}, {4, 20}, {4, 30} map can be stored like {4, 4, 4}, {10, 20, 30} or {4, 4, 4}, {30, 20, 10}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The order of values depends on the order of insertion, as specified in [associative.reqmts.general]/68.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. To me it looks not useful if multimap with the same pairs might happen inequal

Also
- move-construct underlying iterators in the constructors of
`flat_(multi)map`'s iterators;
- fix and test `flat_(multi)map`'s comparison;
- remove possibly pessimizing `reserve`, with signedness adjusted; and
- simply `vector<T, allocator<T>>` to `vector<T>`.
@StephanTLavavej StephanTLavavej added bug Something isn't working flat_meow C++23 container adaptors labels May 7, 2024
@StephanTLavavej StephanTLavavej self-assigned this May 7, 2024
@StephanTLavavej StephanTLavavej merged commit 5b10538 into microsoft:feature/flat_map May 11, 2024
@StephanTLavavej
Copy link
Member

Thanks for flattening away these issues! 🚂 🥞 😹

@frederick-vs-ja frederick-vs-ja deleted the unwrap-pairing-iter branch May 12, 2024 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working flat_meow C++23 container adaptors

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants