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

Fix clang conversion warnings in string_util::split_string_list #1207

Merged

Conversation

bluetarpmedia
Copy link
Contributor

The string_util::split_string_list function in cpp2util.h causes clang to emit some conversion warnings:

warning: implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long') [-Wsign-conversion]
  395 |         while (pos < std::ssize(str) && !is_id_char(str[pos])) {
      |                                                     ~~~ ^~~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:401:56: warning: implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long') [-Wsign-conversion]
  401 |         while (pos < std::ssize(str) && is_id_char(str[pos])) {
      |                                                    ~~~ ^~~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:407:52: warning: implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long') [-Wsign-conversion]
  407 |             ret.emplace_back(str.substr(start, pos - start));
      |                                  ~~~~~~        ~~~~^~~~~~~
raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:407:41: warning: implicit conversion changes signedness: 'int' to 'size_type' (aka 'unsigned long') [-Wsign-conversion]
  407 |             ret.emplace_back(str.substr(start, pos - start));
      |                                  ~~~~~~ ^~~~~

Repro here

This PR fixes those warnings by changing the type of start and pos.

@hsutter
Copy link
Owner

hsutter commented Aug 5, 2024

Thanks! I've updated it to keep the signedness of the original, because I'm trying to follow my own advice of using signed sizes/indexes.

I'll merge it this way... but if this changed version doesn't resolve the problem, please open a new PR. Again, thanks!

@hsutter hsutter merged commit 630e61a into hsutter:main Aug 5, 2024
27 of 28 checks passed
@bluetarpmedia
Copy link
Contributor Author

Unfortunately the clang warnings remain. The warnings are emitted in 2 places when a signed integer is supplied to:

  • the index for str[]
  • the arguments to the substr function call

Here's a repro showing the warnings with the updated function.

I also prefer using signed integer types but they pose an annoying problem with clang when also following the best practice advice to compile with high warning levels!

Here's a version that keeps the signed indices and also uses static_cast but it gets ugly quickly. (Also it casts to size_t rather than the size_type for brevity.)

auto split_string_list(std::string_view str)
    -> std::vector<std::string_view>
{
    std::vector<std::string_view> ret;

    auto is_id_char = [](char c) { 
        return std::isalnum(c) || c == '_';
    };

    auto pos = decltype(std::ssize(str)){ 0 };
    while( pos < std::ssize(str) ) {
        //  Skip non-alnum
        while (pos < std::ssize(str) && !is_id_char(str[static_cast<size_t>(pos)])) {
            ++pos;
        }
        auto start = pos;

        //  Find the end of the current component
        while (pos < std::ssize(str) && is_id_char(str[static_cast<size_t>(pos)])) {
            ++pos;
        }

        //  Add nonempty substring to the vector
        if (start < pos) {
            ret.emplace_back(str.substr(static_cast<size_t>(start), static_cast<size_t>(pos) - static_cast<size_t>(start)));
        }
    }

    return ret;
}

@hsutter
Copy link
Owner

hsutter commented Aug 6, 2024

I also prefer using signed integer types but they pose an annoying problem with clang when also following the best practice advice to compile with high warning levels!

Argh. I thought I did this all the time in cppfront though... does Clang trunk also complain about the rest of cppfront at high warning levels? And is there a specific warning here that should be disabled (e.g., I do disable a few noisy ones at the top of common.h)?

@gregmarr
Copy link
Contributor

gregmarr commented Aug 6, 2024

raw.githubusercontent.com/hsutter/cppfront/main/include/cpp2util.h:395:57: warning: implicit conversion changes signedness: 'decltype(std::ssize(str))' (aka 'long') to 'size_type' (aka 'unsigned long') [-Wsign-conversion]
  395 |         while (pos < std::ssize(str) && !is_id_char(str[pos])) {
      |                                                     ~~~ ^~~

@bluetarpmedia
Copy link
Contributor Author

I created #1212 to hopefully resolve this.

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 this pull request may close these issues.

3 participants