Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add public Boost headers explicitly #27783

Merged
merged 2 commits into from Jun 12, 2023
Merged

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 30, 2023

To check symbols in the code base, run:

git grep boost::multi_index::identity
git grep boost::multi_index::indexed_by
git grep boost::multi_index::tag
git grep boost::make_tuple

Hoping on the absence of conflicts with top-prio PRs :)

@DrahtBot
Copy link
Contributor

DrahtBot commented May 30, 2023

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
ACK MarcoFalke, TheCharlatan

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:

  • #27425 (test: move remaining rand code from util/setup_common to util/random by jonatack)
  • #27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)

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.

test/lint/lint-includes.py Outdated Show resolved Hide resolved
"boost/multi_index/ordered_index.hpp",
"boost/multi_index/sequenced_index.hpp",
"boost/multi_index/tag.hpp",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect to see boost/multi_index_container.hpp removed from this list as part of this PR, but it looks like it's still used:

 $ git grep multi_index_container.hpp origin/pr/27783
origin/pr/27783:node/miner.h:#include <boost/multi_index_container.hpp>
origin/pr/27783:txmempool.h:#include <boost/multi_index_container.hpp>
origin/pr/27783:txrequest.cpp:#include <boost/multi_index_container.hpp>

Is there some complication with those 3 that prevents us from using the specific headers rather than the umbrella one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I misremembered how multi_index_container.hpp worked. I was thinking it was just a dumb list of other includes, but it's more than that.

Still, does using multi_index_container_fwd.hpp help at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, does using multi_index_container_fwd.hpp help at all?

No, it doesn't:

./txmempool.h:406:29: error: field ‘mapTx’ has incomplete type ‘CTxMemPool::indexed_transaction_set’ {aka ‘boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee> > >’}
  406 |     indexed_transaction_set mapTx GUARDED_BY(cs);
      |   

The `BOOST_ASSERT` macro is defined in the `boost/assert.hpp` header.
This change allows to skip `#include <boost/assert.hpp>`.
@hebasto
Copy link
Member Author

hebasto commented May 31, 2023

Taken @MarcoFalke's suggestion.

@maflcko
Copy link
Member

maflcko commented May 31, 2023

lgtm ACK 2484cac

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2484cac

@fanquake fanquake merged commit 361a0c0 into bitcoin:master Jun 12, 2023
16 checks passed
@hebasto hebasto deleted the 230530-boost branch June 12, 2023 16:17
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2023
fanquake added a commit to bitcoin-core/gui that referenced this pull request Jun 22, 2023
… linter

28fff06 test: Make linter to look for `BOOST_ASSERT` macros (Hennadii Stepanov)
47fe551 test: Kill `BOOST_ASSERT` (Hennadii Stepanov)

Pull request description:

  One of the goals of bitcoin/bitcoin#27783 was to get rid of the `BOOST_ASSERT` macros instead of including the `boost/assert.hpp` headers. See bitcoin/bitcoin#27783 (comment).

  It turns out that a couple of those macros sneaked into the codebase in bitcoin/bitcoin#27790.

  This PR makes the linter guard against new instances of the `BOOST_ASSERT` macros and replaces the current ones.

ACKs for top commit:
  kevkevinpal:
    ACK [28fff06](bitcoin/bitcoin@28fff06)
  stickies-v:
    ACK 28fff06
  TheCharlatan:
    ACK 28fff06

Tree-SHA512: 371f613592cf677afe0196d18c83943c6c8f1e998f57b4ff3ee58bfeff8636e4dac1357840d8611b4f7b197def94df10fe1a8ca3282b00b7b4eff4624552dda8
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 22, 2023
28fff06 test: Make linter to look for `BOOST_ASSERT` macros (Hennadii Stepanov)
47fe551 test: Kill `BOOST_ASSERT` (Hennadii Stepanov)

Pull request description:

  One of the goals of bitcoin#27783 was to get rid of the `BOOST_ASSERT` macros instead of including the `boost/assert.hpp` headers. See bitcoin#27783 (comment).

  It turns out that a couple of those macros sneaked into the codebase in bitcoin#27790.

  This PR makes the linter guard against new instances of the `BOOST_ASSERT` macros and replaces the current ones.

ACKs for top commit:
  kevkevinpal:
    ACK [28fff06](bitcoin@28fff06)
  stickies-v:
    ACK 28fff06
  TheCharlatan:
    ACK 28fff06

Tree-SHA512: 371f613592cf677afe0196d18c83943c6c8f1e998f57b4ff3ee58bfeff8636e4dac1357840d8611b4f7b197def94df10fe1a8ca3282b00b7b4eff4624552dda8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants