validationinterface: make MainSignalsInstance() a class, drop unused forward declarations #25067

pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:make-MainSignalsInstance-a-class-and-move-to-header-file changing 2 files +14 −14
  1. jonatack commented at 10:25 AM on May 5, 2022: member

    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 3a19fed9db5 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 d6815a23131 to become class-like:

  2. jonatack commented at 10:27 AM on May 5, 2022: member

    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.

  3. jonatack force-pushed on May 5, 2022
  4. DrahtBot added the label Validation on May 5, 2022
  5. jonatack force-pushed on May 5, 2022
  6. DrahtBot commented at 2:11 PM on May 5, 2022: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  7. jonatack force-pushed on May 5, 2022
  8. in src/validationinterface.h:187 in 1949586afc outdated 
     183 | +/**
 184 | + * MainSignalsInstance manages a list of shared_ptr<CValidationInterface>
 185 | + * callbacks.
 186 | + *
 187 | + * A std::unordered_map is used to track what callbacks are currently
 188 | + * registered, and a std::list is to used to store the callbacks that are
    pk-b2 commented at 4:53 PM on May 6, 2022:

    nit:

     * registered, and a std::list is used to store the callbacks that are
    jonatack commented at 5:02 PM on May 6, 2022:

    Thanks, fixed.

  9. pk-b2 approved
  10. pk-b2 commented at 4:53 PM on May 6, 2022: none

    ACK 1949586afc2f57061d27b87bf48d40861b8a4767

  11. jonatack force-pushed on May 6, 2022
  12. w0xlt approved
  15. MarcoFalke commented at 5:21 AM on May 7, 2022: member

    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.

  16. jonatack renamed this:
    validation, refactor: make MainSignalsInstance() a class, move to h
    validationinterface: make MainSignalsInstance() a class, drop unused forward declarations
    on May 9, 2022
  17. jonatack commented at 2:06 PM on May 9, 2022: member

    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.

  18. jonatack force-pushed on May 9, 2022
  19. in src/validationinterface.h:22 in f5afc238b4 outdated 
      17 | @@ -17,9 +18,7 @@ class BlockValidationState;
  18 |  class CBlock;
  19 |  class CBlockIndex;
  20 |  struct CBlockLocator;
  21 | -class CConnman;
  22 |  class CValidationInterface;
  23 | -class uint256;
    MarcoFalke commented at 2:17 PM on May 9, 2022:

    For reference, with

    diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index e64af2ad5..58661ff4c 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b.sh
@@ -35,14 +35,13 @@ if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
 fi
 
 if [ "${RUN_TIDY}" = "true" ]; then
-  export P_CI_DIR="${BASE_BUILD_DIR}/bitcoin-$HOST/src/"
-  CI_EXEC run-clang-tidy "${MAKEJOBS}"
   export P_CI_DIR="${BASE_BUILD_DIR}/bitcoin-$HOST/"
   CI_EXEC "python3 ${DIR_IWYU}/include-what-you-use/iwyu_tool.py"\
           " src/compat"\
           " src/init"\
           " src/rpc/fees.cpp"\
           " src/rpc/signmessage.cpp"\
+          " src/validationinterface.cpp"\
           " -p . ${MAKEJOBS} -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=${BASE_BUILD_DIR}/bitcoin-$HOST/contrib/devtools/iwyu/bitcoin.core.imp"
 fi

    I get:

    validationinterface.h should add these lines:
#include <stddef.h>                  // for size_t
#include <stdint.h>                  // for uint64_t
#include "threadsafety.h"            // for LOCKS_EXCLUDED

validationinterface.h should remove these lines:
- class CConnman;  // lines 20-20
- class uint256;  // lines 22-22

validationinterface.cpp should add these lines:
#include <assert.h>                  // for assert
#include <ext/alloc_traits.h>        // for __alloc_traits<>::value_type
#include <iterator>                  // for next
#include <list>                      // for list<>::iterator, _List_iterator
#include <vector>                    // for vector
#include "attributes.h"              // for LIFETIMEBOUND
#include "sync.h"                    // for AssertLockNotHeldInternal, LOCK
#include "uint256.h"                 // for uint256
    jonatack commented at 2:37 PM on May 9, 2022:

    Thanks. Dropped the header changes.

  20. MarcoFalke commented at 2:18 PM on May 9, 2022: member

    second commit looks wrong:

    • "enter the commit message for your changes. Lines starting"
    • missing includes (see iwyu)
  21. MarcoFalke commented at 2:19 PM on May 9, 2022: member

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

  22. jonatack force-pushed on May 9, 2022
  23. jonatack commented at 2:38 PM on May 9, 2022: member

    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.

  24. w0xlt approved
  26. promag commented at 3:59 PM on May 9, 2022: member

    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.

  27. refactor: make MainSignalsInstance() a class
    and use Doxygen documentation for it, per our developer notes.

Context:

MainSignalsInstance was created in 3a19fed9db5 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 d6815a23131 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.
    23854f8402
  28. refactor: remove unused forward declarations in validationinterface.h 2aaec2352d
  29. scripted-diff: Rename MainSignalsInstance() class to MainSignalsImpl()
    ```
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src test doc | xargs sed -i "s/$1/$2/g"; }
s 'MainSignalsInstance' 'MainSignalsImpl'
-END VERIFY SCRIPT-
    ca1ac1f0e0
  30. jonatack commented at 4:40 PM on May 9, 2022: member

    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.

  31. jonatack force-pushed on May 9, 2022
  32. promag commented at 5:30 PM on May 9, 2022: member

    Code review ACK ca1ac1f0e0fbabbe97666aca94024afb8104da06.

  33. danielabrozzoni approved
  34. danielabrozzoni commented at 1:06 PM on May 15, 2022: contributor

    Code review ACK ca1ac1f0e0fbabbe97666aca94024afb8104da06

  35. furszy commented at 2:59 PM on May 15, 2022: member

    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.

  36. MarcoFalke merged this on May 16, 2022
  37. MarcoFalke closed this on May 16, 2022
  38. MarcoFalke removed the label Validation on May 16, 2022
  39. MarcoFalke added the label Refactoring on May 16, 2022
  40. jonatack deleted the branch on May 16, 2022
  41. jonatack commented at 4:18 PM on May 16, 2022: member

    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.

  42. sidhujag referenced this in commit e519628c87 on May 28, 2022
  43. Fabcien referenced this in commit 8d4c3f016b on Mar 10, 2023
  44. DrahtBot locked this on May 16, 2023
Contributors
jonatackDrahtBotpk-b2w0xltMarcoFalkepromagdanielabrozzonifurszy
Labels
Refactoring
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me