search

boostorg / unordered

Boost.org unordered module
http://boost.org/libs/unordered
Boost Software License 1.0
62 stars 55 forks source link

Fix `operator->` for `local_iterator`s #220

Closed vslashg closed 9 months ago

vslashg commented 10 months ago

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

pdimov commented 10 months ago

What kind of compile break does it cause?

codecov[bot] commented 10 months ago

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 [![Impacted file tree graph](https://app.codecov.io/gh/boostorg/unordered/pull/220/graphs/tree.svg?width=650&height=150&src=pr&token=ZqRPZlJZ5N&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg)](https://app.codecov.io/gh/boostorg/unordered/pull/220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg) ```diff @@ Coverage Diff @@ ## develop #220 +/- ## ======================================== Coverage 98.07% 98.07% ======================================== Files 143 143 Lines 19695 19705 +10 ======================================== + Hits 19316 19326 +10 Misses 379 379 ``` | [Files](https://app.codecov.io/gh/boostorg/unordered/pull/220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg) | Coverage Δ | | |---|---|---| | [include/boost/unordered/detail/fca.hpp](https://app.codecov.io/gh/boostorg/unordered/pull/220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg#diff-aW5jbHVkZS9ib29zdC91bm9yZGVyZWQvZGV0YWlsL2ZjYS5ocHA=) | `99.66% <100.00%> (+<0.01%)` | :arrow_up: | | [test/unordered/compile\_tests.hpp](https://app.codecov.io/gh/boostorg/unordered/pull/220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg#diff-dGVzdC91bm9yZGVyZWQvY29tcGlsZV90ZXN0cy5ocHA=) | `100.00% <100.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/boostorg/unordered/pull/220?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/boostorg/unordered/pull/220?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg). Last update [1c0e54e...6cdf745](https://app.codecov.io/gh/boostorg/unordered/pull/220?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg).
vslashg commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

Problem fixed in #221.