tcbrindle / flux

A C++20 library for sequence-orientated programming
https://tristanbrindle.com/flux/
Boost Software License 1.0
484 stars 29 forks source link

move of `loc` in assert.hpp causes clang-tidy error #176

Closed braxtons12 closed 6 months ago

braxtons12 commented 6 months ago

Title says it all, exact output is as below:

This is with clang-tidy 16 and 17

/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/assert.hpp:92:71: error: Moved-from object 'loc' of type 'std::source_location' is moved [clang-analyzer-cplusplus.Move,-warnings-as-errors]
            assert_fn{}(idx < limit, "out-of-bounds sequence access", std::move(loc));
                                                                      ^
/Users/runner/work/hyperion_assert/hyperion_assert/src/assert/detail/parser.cpp:156:9: note: Loop condition is true.  Entering loop body
        for(auto cur = flux::first(tokens); !flux::is_last(tokens, cur); flux::inc(tokens, cur)) {
        ^
/Users/runner/work/hyperion_assert/hyperion_assert/src/assert/detail/parser.cpp:157:27: note: Calling 'read_at_fn::operator()'
            auto& token = flux::read_at(tokens, cur);
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/sequence_access.hpp:42:[16](https://github.com/braxtons12/hyperion_assert/actions/runs/8270441336/job/22627969323#step:7:17): note: Calling 'sequence_traits::read_at'
        return traits_t<Seq>::read_at(seq, cur);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/default_impls.hpp:193:9: note: Calling 'indexed_bounds_check_fn::operator()'
        indexed_bounds_check(idx, size(self));
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/assert.hpp:83:13: note: Assuming the condition is true
        if (!std::is_constant_evaluated()) {
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/assert.hpp:83:9: note: Taking true branch
        if (!std::is_constant_evaluated()) {
        ^
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/assert.hpp:85:43: note: Left side of '&&' is false
            if (__builtin_constant_p(idx) && __builtin_constant_p(limit)) {
                                          ^
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/assert.hpp:91:66: note: Object 'loc' of type 'std::source_location' is left in a valid but unspecified state after move
            assert_fn{}(idx >= T{0}, "index cannot be negative", std::move(loc));
                                                                 ^~~~~~~~~~~~~~
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/assert.hpp:92:25: note: Assuming 'idx' is >= 'limit'
            assert_fn{}(idx < limit, "out-of-bounds sequence access", std::move(loc));
                        ^~~~~~~~~~~
/Users/runner/work/hyperion_assert/hyperion_assert/build/env-cc-release/_deps/flux-src/include/flux/core/assert.hpp:92:71: note: Moved-from object 'loc' of type 'std::source_location' is moved
            assert_fn{}(idx < limit, "out-of-bounds sequence access", std::move(loc));
braxtons12 commented 6 months ago

I think the correct thing to do here would be to just not std::move loc in either of those asserts. It's not specified by the standard, but source_location is trivially copyable on all three of the major implementations, and should be on any reasonable implementation as well, in which case moving doesn't actually do anything anyway.

tcbrindle commented 6 months ago

Hi @braxtons12, thanks for the bug report.

The double move is actually pretty obvious in the source code, I'm not sure how I missed it!

https://github.com/tcbrindle/flux/blob/8ae438be0471e46dc7d24077cab37fd209387ccd/include/flux/core/assert.hpp#L91-L92

(As you point out, since source_location is trivially copyable this isn't actually a problem in practice.)

This should be an easy fix :)