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

Prelim fix issue586 (if none better found) #587

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Prelim fix issue586 (if none better found) #587

merged 2 commits into from
Jan 23, 2024

Conversation

ckormanyos
Copy link
Member

@ckormanyos ckormanyos commented Jan 22, 2024

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
Copy link
Member Author

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
Copy link
Collaborator

I'll take a look shortly.

@jzmaddock
Copy link
Collaborator

That's fine, a simpler fix is:

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

which doesn't then require a temporary.

@ckormanyos
Copy link
Member Author

That's fine, a simpler fix is:

Thank you John. It is now cycling in CI.

Copy link

codecov bot commented Jan 23, 2024

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

@@            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 Coverage Δ
...de/boost/multiprecision/detail/functions/trunc.hpp 84.7% <66.7%> (-6.2%) ⬇️

... and 159 files with indirect coverage changes


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 878138a...29ca08c. Read the comment docs.

@ckormanyos
Copy link
Member Author

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

@ckormanyos ckormanyos merged commit 353b309 into develop Jan 23, 2024
71 checks passed
@ckormanyos ckormanyos deleted the issue586 branch January 23, 2024 17:05
@mborland
Copy link
Member

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
Copy link
Member Author

every test suite gets its own coverage run

I appreciate that. Many thanks @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.

3 participants