Refactor: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp by shaavan · Pull Request #23801 · bitcoin/bitcoin

shaavan · 2021-12-17T11:57:46Z

This is a follow-up to PR net: Use type-safe mockable time for peer connection time #23758
This changes the remaining time variable in net_processing.cpp from int64_t to std::chrono::seconds

shaavan · 2021-12-17T11:58:47Z

A gentle ping, @MarcoFalke

maflcko

ack. left some nits

src/net_processing.cpp

shaavan · 2021-12-17T13:14:18Z

Updated from f8b810b8e15b72f5cff4c6e27bea3f6a64fd0c3d to 8e0ba04b2e6e9ecb11b541ecc361c3a9fe5e9fde (pr23801.01 -> pr23801.02)

Changes

Addressed nits, suggested by @MarcoFalke

maflcko

more nits

src/net_processing.cpp

w0xlt

crACK 8e0ba04

hebasto · 2021-12-18T13:25:40Z

Concept ACK.

hebasto

#include <chrono>

?

shaavan · 2021-12-19T13:25:40Z

Updated from 8e0ba04b2e6e9ecb11b541ecc361c3a9fe5e9fde to 1a1fafa4fdcbe7b0d1ef8df466a71bd3e65195d3 (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 · 2021-12-19T13:29:33Z

@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 · 2021-12-19T13:39:33Z

Updated from 1a1fafa4fdcbe7b0d1ef8df466a71bd3e65195d3 to d5207c2b75c33a720879ed1e43f3db1156cdcce8
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

ACK d5207c2.

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

hebasto

ACK d5207c2b75c33a720879ed1e43f3db1156cdcce8

src/net_processing.cpp

…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 · 2021-12-20T06:06:01Z

Updated from d5207c2b75c33a720879ed1e43f3db1156cdcce8 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 · 2021-12-20T07:50:15Z

ACK 92082ea

hebasto

re-ACK 92082ea

hebasto · 2021-12-20T10:09:02Z

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

src/net_processing.cpp

maflcko · 2021-12-20T11:00:27Z

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

… to std…

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

maflcko approved these changes Dec 17, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot added P2P Refactoring labels Dec 17, 2021

shaavan force-pushed the time_in_seconds_refactor branch from f8b810b to 8e0ba04 Compare December 17, 2021 13:13

maflcko approved these changes Dec 17, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

w0xlt approved these changes Dec 17, 2021

View reviewed changes

hebasto reviewed Dec 18, 2021

View reviewed changes

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

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

stratospher reviewed Dec 19, 2021

View reviewed changes

hebasto approved these changes Dec 19, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

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 force-pushed the time_in_seconds_refactor branch from d5207c2 to 92082ea Compare December 20, 2021 06:04

hebasto approved these changes Dec 20, 2021

View reviewed changes

maflcko reviewed Dec 20, 2021

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

maflcko merged commit 3ec8f9f into bitcoin:master Dec 20, 2021

maflcko reviewed Dec 20, 2021

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

shaavan deleted the time_in_seconds_refactor branch December 20, 2021 10:59

maflcko reviewed Dec 20, 2021

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

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 mentioned this pull request Dec 21, 2021

Refactor: Changes time variables from int to chrono #23832

Merged

bitcoin locked and limited conversation to collaborators Dec 20, 2022

Conversation

shaavan commented Dec 17, 2021

Uh oh!

shaavan commented Dec 17, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Dec 17, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 18, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Dec 19, 2021

Uh oh!

hebasto commented Dec 19, 2021

Uh oh!

shaavan commented Dec 19, 2021

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shaavan commented Dec 20, 2021

Uh oh!

naumenkogs commented Dec 20, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants