Page MenuHomePhabricator

[backport#15288] circular-dependencies: Avoid treating some .h/.cpp files as a unit
ClosedPublic

Authored by majcosta on Apr 28 2020, 06:14.

Details

Summary

This avoids a bogus circular dependency error in the next commit:

interfaces/chain -> interfaces/wallet -> wallet/wallet -> interfaces/chain

Which is incorrect, because interfaces/chain.cpp depends only on the
interfaces/wallet.h file, not the interfaces/wallet.cpp file, and it is
wrong to treat these as a unit. Inside the interfaces directory, .h files
contain abstract class definitions and .cpp files contain implementations of
those classes, so you don't need to link against .cpp files if you're only
using the abstract class definition in the .h file.

An alternative fix might be to rename all the cpp files in the interfaces
directory like: chain.cpp->chain_impl.cpp, node.cpp->node_impl.cpp. But just
getting the linter to treat these files as independent dependencies seemed
like it would allow keeping code organization straightforward and avoiding
the need to rename things.

318f41fb2cae0a46b4e4be49156562b8ed640f0c (Russell Yanofski)


Depends on D5866

This is a partial backport of Core PR15288

Test Plan
cmake .. -GNinja -DENABLE_WERROR=ON -DCMAKE_BUILD_TYPE=Debug -DBUILD_BITCOIN_WALLET=OFF
ninja check-all
cmake .. -GNinja -DENABLE_WERROR=ON -DCMAKE_BUILD_TYPE=Debug
ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

deadalnix requested changes to this revision.Apr 28 2020, 13:46
deadalnix added a subscriber: deadalnix.

What part of the PR is this?

This revision now requires changes to proceed.Apr 28 2020, 13:46

added missing commit hash to summary.

This revision is now accepted and ready to land.Apr 28 2020, 15:00