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

sign-conversion warning in catch_tostring.hpp #2545

Closed
Teskann opened this issue Oct 8, 2022 · 2 comments · Fixed by #2546
Closed

sign-conversion warning in catch_tostring.hpp #2545

Teskann opened this issue Oct 8, 2022 · 2 comments · Fixed by #2546

Comments

@Teskann
Copy link
Contributor

Teskann commented Oct 8, 2022

Describe the bug

Hello, the latest devel of Catch2 raises a warning on g++ 12.

conversion to ‘std::size_t’ {aka ‘long unsigned int’} from ‘long int’ may change the sign of the result

This warning was introduced a few days ago by PR #2540

File catch_tostring.hpp line 46:

inline std::size_t catch_strnlen(const char *str, std::size_t n) {
    auto ret = std::char_traits<char>::find(str, n, '\0');
    if (ret != nullptr) {
        return ret - str;  // <-- Here !
    }
    return n;
}

Expected behavior

Catch2 builds without warnings.

Reproduction steps

Build Catch2 with g++ .

Platform information:
g++ version 12.1.0 (Ubuntu 12.1.0-2ubuntu1~22.04)

Possible fixes

  • Change the return type of catch_strnlen to std::ptrdiff_t. This would prevent to return n without a cast to std::ptrdiff_t.
  • Explicitly cast the result to std::size_t.
return static_cast<std::size_t>(ret - str);

This should be relatively safe as str should never be greater than ret. In my opinion this is the way to go.

@rkaminsk what do you think ?

@rkaminsk
Copy link
Contributor

rkaminsk commented Oct 8, 2022

The code safe. We could add a static cast to silence the warning.

@horenmar
Copy link
Member

horenmar commented Oct 8, 2022

static cast is the correct option, ptrdiff return type would just force the cast in callers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants