validationinterface: make MainSignalsInstance() a class, drop unused forward declarations by jonatack · Pull Request #25067 · bitcoin/bitcoin

jonatack · 2022-05-05T10:25:59Z

Make MainSignalsInstance a class, rename it to MainSignalsImpl, use Doxygen documentation for it, and remove no longer used forward declarations in src/validationinterface.h.

MainSignalsInstance was created in 3a19fed and originally was a collection of boost::signals methods moved to validationinterface.cpp, in order to no longer need to include boost/signals in validationinterface.h.

MainSignalsInstance then evolved in d6815a2 to become class-like:

C.8: Use class rather than struct if any member is non-public
C.2: Use class if the class has an invariant; use struct if the data members can vary independently
A class has the advantage of default private access, as opposed to public for a struct.

jonatack · 2022-05-05T10:27:08Z

Most of the diff is move-only and is easily reviewed with git options like --ignore-all-space -U8 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space.

DrahtBot · 2022-05-05T14:11:30Z

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

Conflicts

No conflicts as of last run.

src/validationinterface.h

pk-b2

ACK 1949586afc2f57061d27b87bf48d40861b8a4767

w0xlt

ACK 2911d94

pk-b2 · 2022-05-06T21:11:45Z

ACK 2911d94

maflcko · 2022-05-07T05:21:41Z

Is there a reason to move it to the header? This will make the header heavier and increase the includes, both of which theoretically result in slower builds.

jonatack · 2022-05-09T14:06:17Z

Thanks @MarcoFalke, you're right; there's no reason to and the forward class declaration is lighter. Repushed with the class remaining in the implementation, and removed two forward declarations in the header file that are no longer used.

maflcko

second commit looks wrong:

"enter the commit message for your changes. Lines starting"
missing includes (see iwyu)

src/validationinterface.h

maflcko · 2022-05-09T14:19:53Z

Not sure about fixing includes manually now that we have iwyu. Seems wasteful of review resources and is likely still wrong (as seen here).

jonatack · 2022-05-09T14:38:16Z

Not sure about fixing includes manually now that we have iwyu. Seems wasteful of review resources and is likely still wrong (as seen here).

Yes, I hesitated on it and agree. Dropped the headers.

w0xlt

reACK c05ee1e

promag

Code review ACK c05ee1e8440f7e1030a5d9597cb6963b99fe4051.

nit, would use refactor: prefix instead, like:

refactor: make MainSignalsInstance a class
refactor: remove unused forward declarations

2nd nit, I wouldn't mind dropping Instance from MainSignalsInstance. Maybe rename it to MainSignalsImpl as it looks like CMainSignals follows pimpl.

and use Doxygen documentation for it, per our developer notes. Context: MainSignalsInstance was created in 3a19fed and originally was a struct collection of boost::signals methods moved to validationinterface.cpp, in order to no longer need to include boost/signals in validationinterface.h. MainSignalsInstance then evolved in d6815a2 to remove boost/signals2 and became class-like. [C.8: Use class rather than struct if any member is non-public](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-class) [C.2: Use class if the class has an invariant; use struct if the data members can vary independently](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently) A class also has the advantage of default private access, as opposed to public for a struct.

``` -BEGIN VERIFY SCRIPT- s() { git grep -l "$1" src test doc | xargs sed -i "s/$1/$2/g"; } s 'MainSignalsInstance' 'MainSignalsImpl' -END VERIFY SCRIPT-

jonatack · 2022-05-09T16:40:59Z

nit, would use refactor: prefix instead, like:

2nd nit, I wouldn't mind dropping Instance from MainSignalsInstance. Maybe rename it to MainSignalsImpl as it looks like CMainSignals follows pimpl.

Done. Grepping for MainSignals would yield many other results, so renamed to MainSignalsImpl as proposed.

promag

Code review ACK ca1ac1f.

danielabrozzoni

Code review ACK ca1ac1f

furszy · 2022-05-15T14:59:31Z

Code ACK ca1ac1f

Tiny nit for the renaming ca1ac1f:
Isn't the "main" word in MainSignalsImpl coming from the previous monolithic main.cpp file existence?. Probably could be changed for something else as now the MainSignalsImpl is just a generic container class for the "validation interface" events subscribers.

jonatack · 2022-05-16T16:18:26Z

Isn't the "main" word in MainSignalsImpl coming from the previous monolithic main.cpp file existence?

Hm, perhaps (thanks for reviewing!) I've been reading it to mean "main signals" as in "primary signals" and there are also CMainSignals and GetMainSignals in validationinterface.{h,cpp}, so I don't have a strong opinion.

…a class…

…sImpl Summary: > refactor: make MainSignalsInstance() a class Context: MainSignalsInstance was originally a struct collection of boost::signals methods moved to validationinterface.cpp, in order to no longer need to include boost/signals in validationinterface.h. MainSignalsInstance then evolved to remove boost/signals2 and became class-like. [C.8: Use class rather than struct if any member is non-public](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-class) [C.2: Use class if the class has an invariant; use struct if the data members can vary independently](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently) A class also has the advantage of default private access, as opposed to public for a struct. > refactor: remove unused forward declarations in validationinterface.h > Rename MainSignalsInstance() class to MainSignalsImpl() This is a backport of [[bitcoin/bitcoin#25067 | core#25067]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13265

jonatack force-pushed the make-MainSignalsInstance-a-class-and-move-to-header-file branch from 466a11a to 2b38010 Compare May 5, 2022 10:33

DrahtBot added the Validation label May 5, 2022

jonatack force-pushed the make-MainSignalsInstance-a-class-and-move-to-header-file branch from 2b38010 to 856040e Compare May 5, 2022 14:09

jonatack force-pushed the make-MainSignalsInstance-a-class-and-move-to-header-file branch from 856040e to 1949586 Compare May 5, 2022 14:25

DrahtBot mentioned this pull request May 5, 2022

Strengthen thread safety assertions #24931

Merged

pk-b2 reviewed May 6, 2022

View reviewed changes

src/validationinterface.h Outdated Show resolved Hide resolved

pk-b2 approved these changes May 6, 2022

View reviewed changes

jonatack force-pushed the make-MainSignalsInstance-a-class-and-move-to-header-file branch from 1949586 to 2911d94 Compare May 6, 2022 17:01

w0xlt approved these changes May 6, 2022

View reviewed changes

jonatack changed the title ~~validation, refactor: make MainSignalsInstance() a class, move to h~~ validationinterface: make MainSignalsInstance() a class, drop unused forward declarations May 9, 2022

jonatack force-pushed the make-MainSignalsInstance-a-class-and-move-to-header-file branch from 2911d94 to f5afc23 Compare May 9, 2022 14:06

maflcko reviewed May 9, 2022

View reviewed changes

src/validationinterface.h Outdated Show resolved Hide resolved

jonatack force-pushed the make-MainSignalsInstance-a-class-and-move-to-header-file branch from f5afc23 to c05ee1e Compare May 9, 2022 14:37

w0xlt approved these changes May 9, 2022

View reviewed changes

promag reviewed May 9, 2022

View reviewed changes

jonatack added 3 commits May 9, 2022 18:33

refactor: remove unused forward declarations in validationinterface.h

2aaec23

scripted-diff: Rename MainSignalsInstance() class to MainSignalsImpl()

ca1ac1f

``` -BEGIN VERIFY SCRIPT- s() { git grep -l "$1" src test doc | xargs sed -i "s/$1/$2/g"; } s 'MainSignalsInstance' 'MainSignalsImpl' -END VERIFY SCRIPT-

jonatack force-pushed the make-MainSignalsInstance-a-class-and-move-to-header-file branch from c05ee1e to ca1ac1f Compare May 9, 2022 16:41

promag reviewed May 9, 2022

View reviewed changes

danielabrozzoni approved these changes May 15, 2022

View reviewed changes

maflcko merged commit 195df1e into bitcoin:master May 16, 2022

maflcko added Refactoring and removed Validation labels May 16, 2022

jonatack deleted the make-MainSignalsInstance-a-class-and-move-to-header-file branch May 16, 2022 08:54

maflcko mentioned this pull request May 23, 2022

refactor: Remove defunct attributes.h includes #25184

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022

Merge bitcoin#25067: validationinterface: make MainSignalsInstance() …

e519628

…a class…

bitcoin locked and limited conversation to collaborators May 16, 2023

Conversation

jonatack commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented May 5, 2022

Uh oh!

DrahtBot commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

pk-b2 left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

pk-b2 commented May 6, 2022

Uh oh!

maflcko commented May 7, 2022

Uh oh!

jonatack commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented May 9, 2022

Uh oh!

jonatack commented May 9, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

furszy commented May 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonatack commented May 5, 2022 •

edited

Loading

DrahtBot commented May 5, 2022 •

edited

Loading

jonatack commented May 9, 2022 •

edited

Loading

jonatack commented May 9, 2022 •

edited

Loading

furszy commented May 15, 2022 •

edited

Loading

jonatack commented May 16, 2022 •

edited

Loading