Skip to content

Handle various conversion warnings #1261

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
merged 5 commits into from
Apr 27, 2025
Merged

Handle various conversion warnings #1261

merged 5 commits into from
Apr 27, 2025

Conversation

ckormanyos
Copy link
Member

@ckormanyos ckormanyos commented Apr 27, 2025

Hi folks,

I'm really happy with this one. After literally years of improvements in our combined work, it is now possible to run parts of Math + Multiprecision unchanged off-the-rack on both PC as well as embedded targets.

The purpose of this PR is to handle a few last warnings found when cross-compiling and running Math + Multiprecision on both PC-based GCC as well as 32-bit embedded ARM(R) bare-metal gcc-arrm-none-eabi.

The microcontroller compiler found several warnings that seemed quite reasonable to deal with.

I used warnings:

WARN_FLAGS  :=  -Wall                             \
                -Wextra                           \
                -Wpedantic                        \
                -Wmain                            \
                -Wundef                           \
                -Wconversion                      \
                -Wsign-conversion                 \
                -Wunused-parameter                \
                -Wuninitialized                   \
                -Wshadow

This PR does not cover warnings in all of Math, nor in all of Multiprecision. But quite a bit of specfun and both cpp_bin_float (which also pulls in cpp_int) as well as cpp_dec_float at $101$ decimal digits + Math have been successfully executed on PC and microcontroller and become relatively warning-free.

For cpp_bin_float and cpp_dec_float small amounts of dynamic RAM were needed (primarily for std::allocator in the std::string template class). So I wrote a tiny one-shot allocator for the global new() function and dispensed with the heap.

The Multirpecision benchmark computes the $101$ decimal digit cube root value of

$$ \left(\frac{123456}{100}\right)^{1/3}\approx10.72763694322831704548693173735276478\ldots $$

The Math benchmark computes a small series of cylindrical Bessel function values for 64-bit ::boost::float64_t

$$ J_{1.23}\left(1/n\right), n=1{\ldots}10 $$

The FLASH/RAM consumptions and timings for the Multiprecision benchmark are shown below (target ARM(R) Cortex(R) M4F STM32F446 from ST-Microelectronics(R) clocked at $168~MHz$). The multiprecision numbers were instantiated with $101$ decimal digits. I also used stubbed functions for some C-language posix primitives such as those pulled in by I/O streaming (needed by Math's exception handling even when exceptions are turned off). I also used RTTI disabled, optimization -O2.

type FLASH RAM time [ms]
cpp_bin_float 97k 6.3k 1.0
cpp_dec_float 80k 26k 2.8

Cc: @jzmaddock and @mborland

@ckormanyos ckormanyos requested review from mborland and jzmaddock and removed request for mborland April 27, 2025 07:21
@ckormanyos
Copy link
Member Author

Oh yeah, you'll also see some warnings handled in other stuff like toms748_solve. These are in there since I also did some warning hunting on Ubuntu using my own extended_complex adaption to comupte $500$ decimal digit non-trivial roots of the complex-valued Riemann-zeta function, another cool project I've benched on for the past ten years or so...

Copy link

codecov bot commented Apr 27, 2025

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.91%. Comparing base (5130435) to head (bc0635f).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
...ath/special_functions/detail/bernoulli_details.hpp 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #1261    +/-   ##
=========================================
  Coverage    93.90%   93.91%            
=========================================
  Files          661      661            
  Lines        54559    54866   +307     
=========================================
+ Hits         51233    51526   +293     
- Misses        3326     3340    +14     
Files with missing lines Coverage Δ
include/boost/math/constants/info.hpp 97.14% <ø> (ø)
include/boost/math/distributions/landau.hpp 95.78% <ø> (ø)
include/boost/math/policies/error_handling.hpp 85.58% <100.00%> (ø)
include/boost/math/policies/policy.hpp 95.65% <ø> (ø)
include/boost/math/special_functions/bessel.hpp 100.00% <ø> (ø)
.../boost/math/special_functions/detail/bessel_jn.hpp 100.00% <100.00%> (ø)
.../boost/math/special_functions/detail/bessel_jy.hpp 100.00% <100.00%> (ø)
...math/special_functions/detail/bessel_jy_series.hpp 100.00% <100.00%> (ø)
...ost/math/special_functions/detail/igamma_large.hpp 100.00% <ø> (ø)
...ost/math/special_functions/detail/lgamma_small.hpp 100.00% <ø> (ø)
... and 10 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5130435...bc0635f. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jzmaddock
Copy link
Collaborator

Thanks Chris, other than my couple of comments, this all looks good to me.

@ckormanyos ckormanyos merged commit 9f03f29 into develop Apr 27, 2025
80 of 81 checks passed
@ckormanyos ckormanyos deleted the cleanup_warns branch April 27, 2025 17:02
@ckormanyos
Copy link
Member Author

ckormanyos commented Apr 27, 2025

OK so I see some LCOV warnings and (in this issue not even dealing with that) I did some corrections.

But I'm not quite sure that we have the code annotations exactly right. I looked 3 times but could not find the root cause of inconsistencies.

When the specfun job that is in the cover YAML uploads the results, there are line/annotation inconsistencies. I repaired some of them and got more coverage and consistency, but we are still probably better than we thought.

I'll track this down in another issue. This is not the proper place to perfect our cover annotations.

Cc: @jzmaddock and @mborland

@jzmaddock
Copy link
Collaborator

OK so I see some LCOV warnings and (in this issue not even dealing with that) I did some corrections.

But I'm not quite sure that we have the code annotations exactly right. I looked 3 times but could not find the root cause of inconsistencies.

When the specfun job that is in the cover YAML uploads the results, there are line/annotation inconsistencies. I repaired some of them and got more coverage and consistency, but we are still probably better than we thought.

I haven't really touched on anything in /detail/ yet and I know there's a lot to look at in there.

What I'm trying to do is only annotate when:

  1. It's confirmed as covered by a test (lets say a multiprecision one) that isn't run with coverage analysis because it's too slow.
  2. When it's a GCOV SNAFU: these are mostly multi-line statements which GCOV just can't get right for some reason, the classic is a function call:
f(a,
  b,  // not covered!!
  c);

Which I change to

f(a, b, c);

even though that often makes things harder to read, or else a table:

T values[] = {
... ,   // non of these lines show up as covered.
};

Where I bracket the whole thing in LCOV_EXCL_START/LCOV_EXCL_STOP annotations

Other than that I'm trying to actually find test cases, and/or check to see if it's actually dead code which can be removed and/or converted to an assert.

@ckormanyos
Copy link
Member Author

ckormanyos commented Apr 28, 2025

Yes John (@jzmaddock), I agree with all that.

It's confirmed as covered by a test (lets say a multiprecision one) that isn't run with coverage analysis because it's too slow.

Regarding this particular phrase pertaining to Multiprecision, ...

I was looking at a few of these. I was wondering what you thought of the idea of making a few selected multiprecision spot values? These could be designed to run quickly, having just a few values each, and still cover some of those Multiprecision-only sequences in specfun. Then they could theoretically get into the coverage report and hopefully not add too much run-time on the cover job.

I could start looking into this if you think it's the way to go.

Cc: @mborland

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