Fix operator-> for local_iterators

[vslashg]

Invoking this operator causes a compile break. This appears to have been caused by b1a9cde.

[pdimov]

What kind of compile break does it cause?

[codecov]

Codecov Report

Merging #220 (6cdf745) into develop (1c0e54e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #220   +/-   ##
========================================
  Coverage    98.07%   98.07%           
========================================
  Files          143      143           
  Lines        19695    19705   +10     
========================================
+ Hits         19316    19326   +10     
  Misses         379      379           

Files	Coverage Δ
include/boost/unordered/detail/fca.hpp	99.66% <100.00%> (+<0.01%)	⬆️
test/unordered/compile_tests.hpp	100.00% <100.00%> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c0e54e...6cdf745. Read the comment docs.

[vslashg]

lit->first fails to compile; operator-> for local iterators attempts to return a pointer to the node rather than a pointer to a value, so any attempt to instantiate this method (e.g by using -> to dereference a local_iterator) fails to compile due to an impossible pointer conversion.

The PR contains a new regression test that will demonstrate the problem.

[joaquintides]

Hi @vslashg, we've confirmed the bug you've spotted. The fix will arrive in Boost 1.84 (but after Boost 1.84, beta, which is due today). Thank you!

[cmazakas]

Thank you for updating the tests!

The fix seems more complicated than it needs to be.

Why not just use:

pointer operator->() const noexcept
{
  return std::addressof(dereference());
}

?

Also, we need to add coverage for .cbegin() so we test the const_local_iterator arm as well. It's fine to only update the unordered_map_test here but I'd like to see some coverage for the unordered_set_test as well.

Personally, what I'd like to do here is update the bucket_tests.cpp file. You don't necessarily have to do a whole new test case, you can just update the existing one there. But I'd really like to see some tests that test more than just "compile fine" which is what the compile_tests are for.

I can get this done but I wanted to offer the chance to iterate on the PR.

[vslashg]

I'm certainly willing to make the changes you suggest, but for such a small change it may make more sense for you to implement the fix. Let me know what you prefer; I'm happy to see this fixed either way.

[cmazakas]

but for such a small change it may make more sense for you to implement the fix.

Sure. This is an opportunity to become a contributor though! I wouldn't pass it up.

For the tests, I think we'd cover our bases if we just proved that local_it.operator->() yields the same address as *local_it. The test suite already tests dereferencing the local iterators in other places like invariants.hpp so we don't need to go overboard here, just that operator->() is well-formed and returns the correct pointer value.

Feel free to ask me any questions about how to run b2 or anything else like that.

[vslashg]

The friction of coming up with a test object such that set_local_it->member was well-formed was what discouraged me from writing a set test. If you're happy with the test using explicit syntax of set_local_it.operator->() as a test invocation then things are much simpler.

I should be able to get to this either today or tomorrow. Thanks!

[joaquintides]

Problem fixed in #221.