Skip to content

Refactor: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp #23801

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
merged 1 commit into from
Dec 20, 2021
Merged

Refactor: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp #23801

merged 1 commit into from
Dec 20, 2021

Conversation

shaavan
Copy link
Contributor

@shaavan shaavan commented Dec 17, 2021

@shaavan
Copy link
Contributor Author

shaavan commented Dec 17, 2021

A gentle ping, @MarcoFalke

maflcko
maflcko approved these changes Dec 17, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ack. left some nits

@shaavan shaavan force-pushed the time_in_seconds_refactor branch from f8b810b to 8e0ba04 Compare December 17, 2021 13:13
@shaavan
Copy link
Contributor Author

shaavan commented Dec 17, 2021

Updated from f8b810b to 8e0ba04 (pr23801.01 -> pr23801.02)

Changes

maflcko
maflcko approved these changes Dec 17, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

more nits

w0xlt
w0xlt approved these changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

crACK 8e0ba04

@hebasto
Copy link
Member

hebasto commented Dec 18, 2021

Concept ACK.

hebasto
hebasto reviewed Dec 18, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

#include <chrono>

?

@shaavan shaavan force-pushed the time_in_seconds_refactor branch from 8e0ba04 to 1a1fafa Compare December 19, 2021 13:23
@shaavan
Copy link
Contributor Author

shaavan commented Dec 19, 2021

Updated from 8e0ba04 to 1a1fafa (pr23801.02 -> pr23801.03)

Addressed comment by @MarcoFalke

Changes:

  • Addressed one nit. Removed unnecessary “in seconds.”
  • Changed m_last_tip_update time from std::chrono::seconds -> std::atomic<std::chrono::seconds>. Made other related changes, accordingly.
  • Renamed time_in_seconds to now under void PeerManagerImpl::CheckForStaleTipAndEvictPeers() definition.

@hebasto. I think including <chrono> in this file would be redundant, as the header banman.h, which this file imports, also imports chrono in it.

@hebasto
Copy link
Member

hebasto commented Dec 19, 2021

@shaavan

@hebasto. I think including <chrono> in this file would be redundant, as the header banman.h, which this file imports, also imports chrono in it.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

  • Rationale: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.

@shaavan shaavan force-pushed the time_in_seconds_refactor branch from 1a1fafa to d5207c2 Compare December 19, 2021 13:37
@shaavan
Copy link
Contributor Author

shaavan commented Dec 19, 2021

Updated from 1a1fafa to d5207c2
Addressed @hebasto comment.

Changes:

  • Added <chrono> in the included header as it is a direct dependency in this file.

Thanks, @hebasto, for enlightening me. I will take care of this in the future.

stratospher
stratospher reviewed Dec 19, 2021
View reviewed changes
Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

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

ACK d5207c2.

nit: On the same note, wouldn't #include <atomic> be needed too?

hebasto
hebasto approved these changes Dec 19, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK d5207c2

@shaavan
Change time variable type to std::chrono::seconds in src/net_processi…
92082ea 
…ng.cpp

- This commit is a followup to commit: 60b579
- This changes the remaining time variable in net_processing.cpp from
int64_t to std::chrono
@shaavan shaavan force-pushed the time_in_seconds_refactor branch from d5207c2 to 92082ea Compare December 20, 2021 06:04
@shaavan
Copy link
Contributor Author

shaavan commented Dec 20, 2021

Updated from d5207c2 to 92082ea (pr23801.03 -> pr23801.04)
Addressed @stratospher and @hebasto comments.

Changes:

  • Added atomic in the included header, as it is a direct dependency.
  • Used the universal initializer auto, wherever chrono_literals were used.

@naumenkogs
Copy link
Member

ACK 92082ea

hebasto
hebasto approved these changes Dec 20, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 92082ea

@hebasto
Copy link
Member

hebasto commented Dec 20, 2021

I've restarted the "macOS 12 native [gui, system sqlite only] [no depends]" Cirrus CI task.

maflcko
maflcko reviewed Dec 20, 2021
View reviewed changes
@maflcko maflcko merged commit 3ec8f9f into bitcoin:master Dec 20, 2021
maflcko
maflcko reviewed Dec 20, 2021
View reviewed changes
@shaavan shaavan deleted the time_in_seconds_refactor branch December 20, 2021 10:59
maflcko
maflcko reviewed Dec 20, 2021
View reviewed changes
@maflcko
Copy link
Member

maflcko commented Dec 20, 2021

re-ack. Left two nits and a follow-up idea.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 20, 2021
Merge bitcoin#23801: Refactor: Change time variable type from int64_t…
9eee2a9 
… to std…
@shaavan shaavan mentioned this pull request Dec 21, 2021
Merged
maflcko pushed a commit that referenced this pull request Jan 6, 2022
Merge #23832: Refactor: Changes time variables from int to chrono
0620957 
fe86eb5 Refactor: Uses c++ init convention for time variables (Shashwat)
6111b0d Refactor: Changes remaining time variable type from int to chrono (Shashwat)

Pull request description:

  This PR is a follow-up to #23801.
  This PR aims to make the following changes to all the time variables in **net_processing.cpp** wherever possible.

  - Convert all time variables to `std::chrono.`
  - Use `chorno::literals` wherever possible.
  - Use `auto` keywords wherever possible.
  - Use `var{val}` convention of initialization.

  This PR also minimizes the number of times, serialization of time `count_seconds(..)` occurs.

ACKs for top commit:
  MarcoFalke:
    re-ACK fe86eb5  🏕

Tree-SHA512: c8684c0c60a11140027e36b6e9706a45ecdeae6b5ba0bf267e50655835daee5e5410e34096a8c4eca005f327caae1ac258cc7b8ba663eab58abf131f6d2f4d42
@bitcoin bitcoin locked and limited conversation to collaborators Dec 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@hebasto hebasto hebasto approved these changes

+3 more reviewers

@maflcko maflcko maflcko approved these changes

@stratospher stratospher stratospher left review comments

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@shaavan @hebasto @naumenkogs @maflcko @laanwj @stratospher @w0xlt @DrahtBot