[go: up one dir, main page]

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sparse_set_iterator::operator[] causes warnings when used with size_t #1134

Closed
jonesmz opened this issue Apr 4, 2024 · 6 comments
Closed
Assignees
Labels
invalid not a bug/not an issue/not whatever you want

Comments

@jonesmz
Copy link
jonesmz commented Apr 4, 2024

[[nodiscard]] constexpr reference operator[](const difference_type value) const noexcept {

Because this function is defined in terms of Container::difference_type, using Container::size_type causes a conversion warning. E.g. std::vector::size_type is std::size_t and std::vector::difference_type is std::ptrdiff_t

Shows up in clang-tidy warnings.

clang-tidy --use-color -p=build /home/runner/work/osp-magnum/osp-magnum/test/tasks/main.cpp
/home/runner/work/osp-magnum/osp-magnum/test/tasks/main.cpp:66:76: warning: narrowing conversion from 'unsigned long' to signed type 'entt::internal::sparse_set_iterator<std::vector<osp::TaskId>>::difference_type' (aka 'long') is implementation-defined [bugprone-narrowing-conversions]
Suppressed 47323 warnings (47310 in non-user code, 12 NOLINT, 1 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
            TaskId const        randomTask  = rExec.tasksQueuedRun.begin()[rRand() % runTasksLeft];
@skypjack skypjack self-assigned this Apr 4, 2024
@skypjack skypjack added the triage pending issue, PR or whatever label Apr 4, 2024
@skypjack
Copy link
Owner
skypjack commented Apr 4, 2024

Interesting. I've clang-tidy enabled and cannot reproduce it though.
Can I ask you what param triggers the error? Maybe it's not the only one.

@jonesmz
Copy link
Author
jonesmz commented Apr 4, 2024

It's this PR: https://github.com/TheOpenSpaceProgram/osp-magnum/pull/269/files , the file test/tasks/main.cpp which is the very bottom-most file in the review (at the time of writing).

The deprecation message says to switch from ::at(index) to ::begin()[index], but i was actually able to switch to simply ::operator[](index) directly.

However, originally (before i just switched to ::operator[](index))when i switched from ::at(index) to ::begin()[index] i got the warning from clang tidy i reported in my initial comment.

The types are:

'entt::internal::sparse_set_iterator<std::vector<osp::TaskId>>::difference_type', where osp::TaskId is an enum-class backed by a uint32_t.

https://github.com/TheOpenSpaceProgram/osp-magnum/blob/a747cab17ae3f5bea02a800c8b9cfbe8a1e345c3/src/osp/tasks/tasks.h#L67-L72

difference_type should be std::ptrdiff_t but apparently cpp reference thinks the only requirement is that it's a signed integer. Regardless, it's being calculated on my platform as long.

rRand is type std::mt19937 &rRand defined here: https://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine

which apparently returns std::uint_fast32_t and i guess on the github actions runner is of type unsigned long.

runTasksLeft is auto const runTasksLeft = rExec.tasksQueuedRun.size(); which i imagine is std::size_t, though i'm not 100% sure what the type of tasksQueuedRun is off of the top of my head.

The full function is here:

https://github.com/TheOpenSpaceProgram/osp-magnum/blob/a747cab17ae3f5bea02a800c8b9cfbe8a1e345c3/test/tasks/main.cpp#L51-L73

since unsigned long can't convert to long without precision loss, we get the warning about narrowing conversions.

My recommendation is that you can either make the functions for sparse_set_iterator templated on the std::integral concept, or you can define two versions, one for signed integers and another for unsigned.

But don't fix this on my account, since ::operator[](index) works for me just fine. I just wanted to report the warning in case you wanted to fix it.

@skypjack
Copy link
Owner
skypjack commented Apr 4, 2024

Ohoo, I see, so the error is at the call site in your case and not in the body of the function, is it?
I got it the other way around initially. This would explain why it doesn't trigger on the CI at least.

@skypjack skypjack added invalid not a bug/not an issue/not whatever you want and removed triage pending issue, PR or whatever labels Apr 5, 2024
@skypjack
Copy link
Owner
skypjack commented Apr 5, 2024

I checked the standard and a few implementations for the major compilers.
As expected, operator[] for random access iterators is always defined in terms of difference_type.
Therefore, I think I'll stick with the current implementation. Thanks for bringing it up though. 👍

@skypjack skypjack closed this as completed Apr 5, 2024
@jonesmz
Copy link
Author
jonesmz commented Apr 5, 2024

Do you mind sharing some of the random access iterators that you were looking at? I'm curious.

@skypjack
Copy link
Owner
skypjack commented Apr 6, 2024

You can check a few implementations here (working draft for the language). Then I checked the MSVC one from within VS (just look into the implementation of a container like the vector) and the ones offered by clang (easily available online). 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid not a bug/not an issue/not whatever you want
Projects
None yet
Development

No branches or pull requests

2 participants