Skip to content

Disable some clang conversion warnings in cpp2util.h and cppfront itself #1212

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

Merged

Conversation

bluetarpmedia
Copy link
Contributor

This PR disables some clang conversion warnings in 2 places:

  • cpp2util.h for Cpp2 users
  • cppfront translation unit when compiling cppfront

The warnings appear when compiling with clang with -Wconversion (as recommend by Jason Turner).

cpp2util.h disables Wsign-conversion (only in that header) to allow warning-free signed integer types for indices and sizes.

The cppfront translation unit (via common.h) disables the above and also Wstring-conversion to enable implicit conversions of string literals to bools for assert(!"message").

Some regression tests are updated because the line numbers have changed in cpp2util.h.


Note: there are some other 64-bit to 32-bit precision loss conversion warnings in cppfront itself (which this PR doesn't address) but I think we should fix them via casts rather than disabling the warning, since that warning may find other issues in the future.

$ clang++ source/cppfront.cpp -std=c++20 -o cppfront -Wall -Wconversion
source/cppfront.cpp:116:64: warning: implicit conversion loses integer precision: 'size_type' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
  116 |                         << std::right << std::setw(total_lines.size())
      |                                          ~~~       ~~~~~~~~~~~~^~~~~~
source/cppfront.cpp:119:64: warning: implicit conversion loses integer precision: 'size_type' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
  119 |                         << std::right << std::setw(total_lines.size())
      |                                          ~~~       ~~~~~~~~~~~~^~~~~~
source/cppfront.cpp:148:67: warning: implicit conversion loses integer precision: 'size_type' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
  148 |                             << std::right << std::setw(total_time.size())
      |                                              ~~~       ~~~~~~~~~~~^~~~~~
3 warnings generated.

cppfront and cpp2util.h use signed integer types for indices and container sizes so disable signed-to-unsigned conversion warnings.

cppfront also uses implicit conversions from string literal to bool for:
`assert(!"message")`
so disable those warnings too.
hsutter added 2 commits August 8, 2024 10:16
Using the answer we recommend for everyone else, so model the right behavior here
@hsutter
Copy link
Owner

hsutter commented Aug 8, 2024

Thanks! I've taken a pass and actually just removed the !"string literal" uses so that we don't need to disable the second Clang warning. And I've also addressed the three .size()-to-int cases you mentioned above so they're also addressed.

This seems to build clean and pass regression, but I don't have Clang 18+ installed -- before merging, please confirm that this looks right to you and still builds clean on the newer Clang with high warnings?

@bluetarpmedia
Copy link
Contributor Author

Looks good, no warnings with clang 18.

@hsutter
Copy link
Owner

hsutter commented Aug 9, 2024

Thanks!

@hsutter hsutter merged commit 658d307 into hsutter:main Aug 9, 2024
29 checks passed
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.

2 participants