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

util: Don't throw in GetTime{Millis,Micros}(). Mark as noexcept. #13382

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jun 3, 2018

  • boost::posix_time::ptime(boost::gregorian::date(1970,1,1) throws bad_year, bad_day_of_month, or bad_day_month (derivatives of std::out_of_range) if the year, month or day are out of range. It is obvious to us that it won't throw at run-time for 1970-01-01, but that is not obvious for the compiler or static analyzers. From a static analyzer perspective it will look like these exceptions can reach all the way up to NodeImpl::appInitMain() where they are unhandled.
  • boost::posix_time::from_time_t(0) is simpler and doesn't throw.
  • Re-use the result of boost::posix_time::from_time_t(0) like we're doing in DecodeDumpTime(…).
  • Mark GetTimeMillis() and GetTimeMicros() as noexcept.

Copy link
Contributor

@qmma70 qmma70 left a comment

Choose a reason for hiding this comment

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

Why not use std::chrono::high_resolution_clock?

@practicalswift
Copy link
Contributor Author

@qmma70 I'm all for removing Boost, but that is something for another PR :-)

@sipa
Copy link
Member

sipa commented Jun 14, 2018

utACK bcdf285db285932365fe6cae17ba5a585cd1a17e

@DrahtBot
Copy link
Contributor

The last travis run for this pull request was 55 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot closed this Jul 29, 2018
@DrahtBot DrahtBot reopened this Jul 29, 2018
@Empact
Copy link
Member

Empact commented Aug 11, 2018

@sipa
Copy link
Member

sipa commented Aug 11, 2018

@Empact What in particular is an argument against noexcept here?

@Empact
Copy link
Member

Empact commented Aug 11, 2018

I guess I'm more wondering: what is the justification for it? If it doesn't help with significant performance optimizations (ala move) and we don't expect it to affect the error performance of the method (because we don't expect it to throw) then why should we apply the treatment? If we should apply it here, should we apply it to most methods? Just a notion.

@practicalswift
Copy link
Contributor Author

@Empact Scott Meyers has this to say about noexcept: http://aristeia.com/EC++11-14/noexcept%202014-03-31.pdf – "Declare noexcept whenever possible" :-)

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 20, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@practicalswift practicalswift force-pushed the dont-throw-in-GetTimeMillis-and-GetTimeMicros branch from bcdf285 to afc97c7 Compare November 5, 2018 12:51
@ken2812221
Copy link
Contributor

utACK afc97c7

@maflcko
Copy link
Member

maflcko commented Dec 3, 2018

Looks like this is made obsolete by something like #9566?

@practicalswift
Copy link
Contributor Author

practicalswift commented Dec 3, 2018

@MarcoFalke Yes, guess so.

Perhaps I should close this PR? The interest in making GetTime{Millis,Micros}() noexcept seems to be limited: only @sipa, @ken2812221 and I have expressed interest in getting this in :-)

@practicalswift practicalswift deleted the dont-throw-in-GetTimeMillis-and-GetTimeMicros branch April 10, 2021 19:36
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants