Skip to content

Use z = std::max(x - y, 0) instead of z = x - y; if (z < 0) z = 0; #9553

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Feb 15, 2017
Merged

Use z = std::max(x - y, 0) instead of z = x - y; if (z < 0) z = 0; #9553

laanwj merged 1 commit into from
Feb 15, 2017

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 14, 2017

Prefer ...

z = std::max(x - y, 0);

... over ...

z = x - y;
if (z < 0)
    z = 0;

... as suggested by @robmcl4.

Please note that the nSinceLastSeen is intentionally skipped since nSinceLastSeen is unused and is being removed as part of #9532.

@practicalswift practicalswift force-pushed the std-max branch 2 times, most recently from 433bb96 to bb35501 Compare January 14, 2017 10:50
maflcko
maflcko reviewed Jan 14, 2017
View reviewed changes
src/qt/coincontroldialog.cpp Outdated
Copy link
Member

@maflcko maflcko Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo this is a step back to before #4234

Copy link
Contributor Author

@practicalswift practicalswift Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Updated version pushed :-)

sipa
sipa reviewed Jan 14, 2017
View reviewed changes
src/addrman.cpp Outdated
Copy link
Member

@sipa sipa Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

micronit: maybe use std::max<int64_t>(nNow - nLastTry, 0) to avoid a cast?

Copy link
Contributor Author

@practicalswift practicalswift Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Fixed and pushed! :-)

src/qt/coincontroldialog.cpp Outdated
Copy link
Member

@sipa sipa Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

@practicalswift practicalswift Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and pushed!

@luke-jr
Copy link
Member

luke-jr commented Jan 14, 2017

I think it's actually more readable the longer way in some cases, but don't care strongly. (I would probably think differently if there was an alias called std::at_least or something.)

@practicalswift practicalswift mentioned this pull request Jan 18, 2017
Merged
jtimon
jtimon reviewed Feb 2, 2017
View reviewed changes
src/addrman.cpp Outdated
Copy link
Contributor

@jtimon jtimon Feb 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not do the same for nSinceLastSeen while you're at it for consistency?
EDIT: Never mind, is being removed in #9532

Copy link
Contributor Author

@practicalswift practicalswift Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtimon See the comment in the PR body:

Please note that the nSinceLastSeen is intentionally skipped since nSinceLastSeen is unused and is being removed as part of #9532.

:-)

@jtimon
Copy link
Contributor

jtimon commented Feb 2, 2017 •
edited
Loading

ACK 660b28b modulo nit.

maflcko
maflcko reviewed Feb 2, 2017
View reviewed changes
src/txmempool.cpp Outdated
Copy link
Member

@maflcko maflcko Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about to get removed as well, so you can drop it here.

Copy link
Contributor Author

@practicalswift practicalswift Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcoFalke What should be dropped? Sorry, didn't catch that :-)

Do you mean that the changes to src/txmempool.cpp should be excluded from this commit?

Copy link
Member

@maflcko maflcko Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything that has "priority" in its name is scheduled to be removed, so this function as well.

@practicalswift
Copy link
Contributor Author

practicalswift commented Feb 3, 2017

@MarcoFalke Fixed and pushed! Looks good? :-)

@jtimon
Copy link
Contributor

jtimon commented Feb 3, 2017

ACK c04f80b

@maflcko
Copy link
Member

maflcko commented Feb 3, 2017 via email

@laanwj
Copy link
Member

laanwj commented Feb 7, 2017

Needs rebase after #9532

@practicalswift
Copy link
Contributor Author

practicalswift commented Feb 7, 2017

@laanwj Thanks for the ping! Rebased and pushed! :-)

@jtimon
Copy link
Contributor

jtimon commented Feb 8, 2017 •
edited
Loading

ACK a47da4b

@practicalswift
Copy link
Contributor Author

practicalswift commented Feb 14, 2017

Anything needed before merge? :-)

@paveljanik
Copy link
Contributor

paveljanik commented Feb 14, 2017

ACK a47da4b

@laanwj laanwj merged commit a47da4b into bitcoin:master Feb 15, 2017
laanwj added a commit that referenced this pull request Feb 15, 2017
@laanwj
Merge #9553: Use z = std::max(x - y, 0) instead of z = x - y; if (z <…
4c69d68 
… 0) z = 0;

a47da4b Use z = std::max(x - y, 0); instead of z = x - y; if (z < 0) z = 0; (practicalswift)
codablock pushed a commit to codablock/dash that referenced this pull request Jan 23, 2018
@laanwj @codablock
Merge bitcoin#9553: Use z = std::max(x - y, 0) instead of z = x - y; …
f2b5560 
…if (z < 0) z = 0;

a47da4b Use z = std::max(x - y, 0); instead of z = x - y; if (z < 0) z = 0; (practicalswift)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@laanwj @andvgal
Merge bitcoin#9553: Use z = std::max(x - y, 0) instead of z = x - y; …
1a5fd0e 
…if (z < 0) z = 0;

a47da4b Use z = std::max(x - y, 0); instead of z = x - y; if (z < 0) z = 0; (practicalswift)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
@laanwj @CryptoCentric
Merge bitcoin#9553: Use z = std::max(x - y, 0) instead of z = x - y; …
2510bfe 
…if (z < 0) z = 0;

a47da4b Use z = std::max(x - y, 0); instead of z = x - y; if (z < 0) z = 0; (practicalswift)
@practicalswift practicalswift deleted the std-max branch April 10, 2021 19:29
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa left review comments

+3 more reviewers

@jtimon jtimon jtimon left review comments

@maflcko maflcko maflcko left review comments

@dcousens dcousens dcousens approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@practicalswift @luke-jr @jtimon @maflcko @laanwj @paveljanik @dcousens @sipa