refactor: move index class members from protected to private #24150

jonatack · 2022-01-25T12:45:21Z

Simplify the index base and child classes and improve encapsulation and clarity by making fully overridden virtual member functions private. In the child classes, this allows having only public and private members. This change also help contributors add virtual functions into the correct section. Also rename the GetName() getters to GetIndexName() for clarity and easier grepping/reviewing.

Some context:

A base class virtual function doesn't need to be accessible to be overridden, but it can only be private if fully overridden, e.g. no inherited base implementation is used by the child classes.

References:

https://en.cppreference.com/w/cpp/language/virtual: "Base::vf does not need to be accessible or visible to be overridden. (Base::vf can be declared private, or Base can be inherited using private inheritance.)"
"An implementation member should be protected if it provides an operation or data that a derived class will need to use in its own implementation. Otherwise, implementation members should be private." - C++ Primer (Lippman, Lajoie, Moo)
Herb Sutter in http://www.gotw.ca/publications/mill18.htm: "Make virtual functions private, and only if derived classes need to invoke the base implementation of a virtual function, make the virtual function protected. Prefer to make interfaces non-virtual using the Template pattern."

DrahtBot · 2022-01-26T00:13:16Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	PastaPastaPasta, aureleoules
Stale ACK	RandyMcMillan, w0xlt, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25494 (indexes: Stop using node internal types by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

PastaPastaPasta

utACK 732fafc, it makes sense to me to make stuff private instead of protected where possible

RandyMcMillan · 2022-02-12T02:17:39Z

commit 7a8d2c0 scripted-diff tACK
Recreated here: https://cirrus-ci.com/build/4937613508345856 (with gsed on macOS)
Tested on macOS (Darwin ₿ 19.6.0 Darwin Kernel Version 19.6.0: x86_64)

RandyMcMillan · 2022-02-12T02:39:59Z

commit 5743fa6 - Approach ACK
https://cirrus-ci.com/build/6508504977506304

NIT: since this is a refactor - Can we bring base.h into clang-format compliance?
https://cirrus-ci.com/build/5869934608646144
commit RandyMcMillan@22f74b8

diff --git a/src/index/base.h b/src/index/base.h
index 9c4d67865a..44d87f8556 100644
--- a/src/index/base.h
+++ b/src/index/base.h
@@ -33,7 +33,7 @@ protected:
      * A locator is used instead of a simple hash of the chain tip because blocks
      * and block index entries may not be flushed to disk until after this database
      * is updated.
-    */
+     */
     class DB : public CDBWrapper
     {
     public:

jonatack · 2022-02-13T17:04:54Z

Thanks for reviewing @RandyMcMillan. It isn't part of the changes here but happy to if I have to repush. Edit: done.

jonatack · 2022-03-09T14:13:05Z

Rebased per git range-diff 7003b6a 732fafc 7785095 and took suggestion in #24150 (comment).

Invalidates previous ACKs by @PastaPastaPasta and @RandyMcMillan; mind re-ACKing?

jonatack · 2022-04-15T09:31:15Z

@PastaPastaPasta and @RandyMcMillan, would you mind re-ACKing here?

PastaPastaPasta

re-utACK

RandyMcMillan · 2022-04-15T22:25:03Z

tACK 7785095

tested on

Darwin ₿ 19.6.0 Darwin Kernel Version 19.6.0: Sun Nov 14 19:58:51 PST 2021; root:xnu-6153.141.50~1/RELEASE_X86_64 x86_64

brunoerg

ACK 7785095

Agreed on updating them to private and changing from GetName to GetIndexName seems better to understand it.

to improve grepping for a more specific function name. -BEGIN VERIFY SCRIPT- sed -i 's/GetName/GetIndexName/g' ./src/index/* -END VERIFY SCRIPT-

and add doxygen comments

and add doxygen comments, and touch up doxygen indentation on one line per review request

and fix some clang formatting

jonatack · 2022-04-28T19:34:45Z

Rebased (see git range-diff dabec990 cff04a7 4fdd404) and added 4fdd404 left over from an unintentional reverted change in #21726.

@PastaPastaPasta, @RandyMcMillan and @brunoerg, mind re-ACKing? Thanks ❤️

brunoerg

re-ACK 4fdd404

src/index/base.h

w0xlt

crACK f5e985b

brunoerg · 2022-05-19T21:08:08Z

re-ACK f5e985b

in the class declaration by access and characteristic for clarity and documentation purposes.

jonatack · 2022-05-20T10:07:25Z

Updated the motivation in the PR description and simplified the last commit by dropping the class-to-struct change, per git diff f5e985b 3f26a36

--- a/src/index/base.h
+++ b/src/index/base.h
@@ -57,8 +57,9 @@ protected:
      * and block index entries may not be flushed to disk until after this database
      * is updated.
      */
-    struct DB : public CDBWrapper
+    class DB : public CDBWrapper
     {
+    public:
         DB(const fs::path& path, size_t n_cache_size,
            bool f_memory = false, bool f_wipe = false, bool f_obfuscate = false);

bvbfan · 2022-06-24T16:23:40Z

src/index/base.h

@@ -26,14 +26,37 @@ struct IndexSummary {
 */
 class BaseIndex : public CValidationInterface
 {
+public:
+    /// Destructor interrupts sync thread if running and blocks until it exits.
+    virtual ~BaseIndex();


You don't need to mark it virtual, there is a bug and it is destructor of CValidationInterface should marked as virtual

DrahtBot · 2022-07-19T21:16:11Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

achow101 · 2022-10-12T18:58:30Z

Are you still working on this?

achow101 · 2022-11-10T17:13:10Z

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

jonatack · 2022-11-10T17:15:04Z

Are you still working on this?

Yes. Might be useful to re-open rather than opening a new one, as there is discussion and some feedback to address.

aureleoules

Concept ACK

fanquake · 2023-02-06T14:58:08Z

Moved to draft for now. Was reopened 3 months ago to continue work / follow up on discussion, but hasn't been touched. Has needed rebase for 6 months.

jonatack · 2023-04-25T11:08:34Z

Closing temporarily so that I can re-open it -- am still interested to work on this.

DrahtBot added Refactoring UTXO Db and Indexes labels Jan 25, 2022

This was referenced Jan 26, 2022

index: Commit MuHash and best block together for coinstatsindex #24138

Merged

Improve Indices on pruned nodes via prune blockers #21726

Merged

PastaPastaPasta approved these changes Feb 1, 2022

View reviewed changes

This was referenced Feb 2, 2022

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

assumeutxo #15606

Closed

DrahtBot added the Needs rebase label Mar 9, 2022

jonatack force-pushed the move-index-class-members-from-protected-to-private branch 2 times, most recently from a1fa9ae to 7785095 Compare March 9, 2022 14:10

DrahtBot removed the Needs rebase label Mar 9, 2022

PastaPastaPasta approved these changes Apr 15, 2022

View reviewed changes

brunoerg approved these changes Apr 18, 2022

View reviewed changes

DrahtBot added the Needs rebase label Apr 26, 2022

jonatack force-pushed the move-index-class-members-from-protected-to-private branch from 7785095 to ef27869 Compare April 28, 2022 14:21

DrahtBot removed the Needs rebase label Apr 28, 2022

jonatack force-pushed the move-index-class-members-from-protected-to-private branch from ef27869 to 6251ba4 Compare April 28, 2022 17:18

jonatack added 6 commits April 28, 2022 19:20

scripted-diff: rename GetName() to GetIndexName() in src/index

51d043f

to improve grepping for a more specific function name. -BEGIN VERIFY SCRIPT- sed -i 's/GetName/GetIndexName/g' ./src/index/* -END VERIFY SCRIPT-

index, refactor: move GetIndexName() getters from protected to private

dcce3ef

and add doxygen comments

index, refactor: move WriteBlock() from protected to private

92856cf

and add doxygen comments, and touch up doxygen indentation on one line per review request

index, refactor: move GetDB() from protected to private

ef0636f

index, refactor: move TxIndex::DB from protected to private

2dc442f

index, refactor: move Init(), CommitInternal() and Rewind() to private

400cac1

index, refactor: move SetBestBlockIndex() to private

4fdd404

and fix some clang formatting

jonatack force-pushed the move-index-class-members-from-protected-to-private branch 2 times, most recently from cff04a7 to 4fdd404 Compare April 28, 2022 17:22

brunoerg approved these changes Apr 28, 2022

View reviewed changes

laanwj reviewed May 9, 2022

View reviewed changes

src/index/base.h Outdated Show resolved Hide resolved

w0xlt approved these changes May 10, 2022

View reviewed changes

index, refactor: document and group BaseIndex virtual functions

3f26a36

in the class declaration by access and characteristic for clarity and documentation purposes.

jonatack force-pushed the move-index-class-members-from-protected-to-private branch from f5e985b to 3f26a36 Compare May 20, 2022 10:05

DrahtBot mentioned this pull request May 25, 2022

CChainState -> Chainstate #24513

Merged

jonatack mentioned this pull request May 27, 2022

wallet: Save wallet scan progress #25036

Merged

This was referenced Jun 10, 2022

wallet: avoid rescans under assumed-valid blocks #23997

Merged

ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) in BaseIndex #25365

Closed

bvbfan reviewed Jun 24, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 29, 2022

indexes: Stop using node internal types #25494

Merged

DrahtBot added the Needs rebase label Jul 19, 2022

achow101 closed this Nov 10, 2022

achow101 reopened this Nov 10, 2022

aureleoules reviewed Nov 11, 2022

View reviewed changes

fanquake marked this pull request as draft February 6, 2023 14:58

jonatack closed this Apr 25, 2023

bitcoin locked and limited conversation to collaborators Apr 24, 2024

refactor: move index class members from protected to private #24150

refactor: move index class members from protected to private #24150

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jonatack commented Mar 9, 2022

Uh oh!

jonatack commented Apr 15, 2022

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Apr 15, 2022

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented May 19, 2022

Uh oh!

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

Uh oh!

Uh oh!

bvbfan Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 19, 2022

Uh oh!

achow101 commented Oct 12, 2022

Uh oh!

achow101 commented Nov 10, 2022

Uh oh!

jonatack commented Nov 10, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 6, 2023

Uh oh!

jonatack commented Apr 25, 2023

Uh oh!

Uh oh!

jonatack commented Jan 25, 2022 •

edited

Loading

DrahtBot commented Jan 26, 2022 •

edited

Loading

RandyMcMillan commented Feb 12, 2022 •

edited

Loading

RandyMcMillan commented Feb 12, 2022 •

edited

Loading

jonatack commented Feb 13, 2022 •

edited

Loading

jonatack commented Apr 28, 2022 •

edited

Loading

jonatack commented May 20, 2022 •

edited

Loading