boostorg / multiprecision

Boost.Multiprecision
Boost Software License 1.0
188 stars 112 forks source link

Prelim fix issue586 (if none better found) #587

Closed ckormanyos closed 6 months ago

ckormanyos commented 6 months ago

The purpose of this PR is to provide a minimalistic fix to #586.

In that issue, we see a phenomenon that may require conversion to number-wrap or non-wrapped type detected in overloads of trunc and lltrunc.

If no better fix is found in this releasce cycle, then this does the trick.

Cc: @jzmaddock John take a look if you find better...

Cc: @mborland Standalone with/without the presence of Math.

Cc: @radj307

ckormanyos commented 6 months ago

It's green-ish with CodeCov upload error (re-running) and 1 timeout on drone.

Hi @jzmaddock do you think the potential fix in this PR is sufficient?

Cc: @mborland

jzmaddock commented 6 months ago

I'll take a look shortly.

jzmaddock commented 6 months ago

That's fine, a simpler fix is:

    if (arg > 0) 
       return floor(arg);
    return ceil(arg);

which doesn't then require a temporary.

ckormanyos commented 6 months ago

That's fine, a simpler fix is:

Thank you John. It is now cycling in CI.

codecov[bot] commented 6 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (878138a) 94.1% compared to head (29ca08c) 84.6%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/boostorg/multiprecision/pull/587/graphs/tree.svg?width=650&height=150&src=pr&token=SDDBym7Pc9&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg)](https://app.codecov.io/gh/boostorg/multiprecision/pull/587?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg) ```diff @@ Coverage Diff @@ ## develop #587 +/- ## ========================================== - Coverage 94.1% 84.6% -9.5% ========================================== Files 273 149 -124 Lines 28523 16332 -12191 ========================================== - Hits 26833 13805 -13028 - Misses 1690 2527 +837 ``` | [Files](https://app.codecov.io/gh/boostorg/multiprecision/pull/587?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg) | Coverage Δ | | |---|---|---| | [...de/boost/multiprecision/detail/functions/trunc.hpp](https://app.codecov.io/gh/boostorg/multiprecision/pull/587?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg#diff-aW5jbHVkZS9ib29zdC9tdWx0aXByZWNpc2lvbi9kZXRhaWwvZnVuY3Rpb25zL3RydW5jLmhwcA==) | `84.7% <66.7%> (-6.2%)` | :arrow_down: | ... and [159 files with indirect coverage changes](https://app.codecov.io/gh/boostorg/multiprecision/pull/587/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/boostorg/multiprecision/pull/587?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/boostorg/multiprecision/pull/587?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg). Last update [878138a...29ca08c](https://app.codecov.io/gh/boostorg/multiprecision/pull/587?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boostorg).
ckormanyos commented 6 months ago

Hi Matt (@mborland) in Multiprecision, uploading Coverage from suite number 1 takes too long and times out all the time, or a lot of the time. We need to eventually look into this.

I can help when we both have a time slot for this...

Cc: @jzmaddock

mborland commented 6 months ago

In math every test suite gets its own coverage run because just doing suite 1 and suite 2 also had issues with timeouts. Porting that logic is likely the easiest way to solve this.

ckormanyos commented 6 months ago

every test suite gets its own coverage run

I appreciate that. Many thanks @mborland