ci, iwyu: Update mappings

hebasto commented at 10:23 am on May 21, 2023: member

This PR reduces false warnings, especially for unit tests.

From the Boost.Test’s point of view, the boost/test/unit_test.hpp header:

0... should be the only header necessary to include to start using the framework

DrahtBot commented at 10:23 am on May 21, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	maflcko

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

Conflicts

No conflicts as of last run.

hebasto force-pushed on May 21, 2023

hebasto marked this as a draft on May 21, 2023

hebasto force-pushed on May 21, 2023

hebasto marked this as ready for review on May 21, 2023

hebasto force-pushed on May 22, 2023

hebasto commented at 8:47 am on May 22, 2023: member

Rebased on top of the merged #27707.

DrahtBot added the label CI failed on May 22, 2023

hebasto commented at 8:21 am on May 29, 2023: member

CI failure seems unrelated.

in contrib/devtools/iwyu/bitcoin.core.imp:42 in cc44f31924 outdated

24+  { include: [ "<boost/test/tools/old/interface.hpp>", public, "<boost/test/unit_test.hpp>", public ] },
25+  { include: [ "<boost/test/tree/test_unit.hpp>", public, "<boost/test/included/unit_test.hpp>", public ] },
26+  { include: [ "<boost/test/unit_test_log.hpp>", public, "<boost/test/unit_test.hpp>", public ] },
27+  { include: [ "<boost/test/unit_test_suite.hpp>", public, "<boost/test/unit_test.hpp>", public ] },
28+  { include: [ "<boost/test/utils/basic_cstring/basic_cstring.hpp>", public, "<boost/test/unit_test.hpp>", public ] },
29+  { include: [ "<boost/test/utils/lazy_ostream.hpp>", public, "<boost/test/unit_test.hpp>", public ] },

maflcko commented at 10:57 am on May 29, 2023:

Would it make sense to upstream those?

fanquake commented at 10:59 am on May 29, 2023:

Was wondering the same about boost-1.75-all.imp and qt5_11.imp, given they are clearly for older versions. Are we going to upstream updates, or just wait until the mappings break?

hebasto commented at 11:04 am on May 29, 2023:

Yes, I did consider upstreaming these changes. It seems make sense to do it after removing all Boost and Qt related warnings for our code, to be sure that the changes are complete.

maflcko commented at 11:25 am on May 29, 2023:

If you want to look for missing ones, there should be at least signals2? See https://api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log with search string “+#include <boos”

hebasto commented at 11:46 am on May 29, 2023:

I’ve skipped non-test Boost stuff intentionally to avoid potential conflicts with other (more important) PRs.

maflcko commented at 11:51 am on May 29, 2023:

contrib/devtools/iwyu/bitcoin.core.imp isn’t modified by anyone else, so shouldn’t cause any (silent) merge conflicts?

hebasto commented at 1:00 pm on May 29, 2023:

If you want to look for missing ones, there should be at least signals2? See api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log with search string “+#include <boos”

Thanks! Done.

contrib/devtools/iwyu/bitcoin.core.imp isn’t modified by anyone else, so shouldn’t cause any (silent) merge conflicts?

You’re right. I meant actually adding/removing boost/signals2/* headers in the code.

maflcko commented at 10:35 am on May 30, 2023:

Same for #include <boost/multi_index/hashed_index.hpp> // for hashed_index_iterator etc … ?

maflcko commented at 9:33 am on September 21, 2023:

Same for #include <boost/multi_index/hashed_index.hpp> // for hashed_index_iterator etc … ?

Are you still working on this?

hebasto commented at 11:27 am on September 21, 2023:

Same for #include <boost/multi_index/hashed_index.hpp> // for hashed_index_iterator etc … ?

Why? boost/multi_index/hashed_index.hpp is a public header. And all IWYU’s suggestions about it look correct.

Or did I miss something?

maflcko commented at 9:31 am on October 20, 2023:

Ah, I think that boost is an implementation detail, so it should be IWYU export ed from the header that exports the boost include.

hebasto commented at 7:14 pm on October 26, 2023:

Do you mean headers in the boost codebase, or in ours?

maflcko commented at 8:26 am on October 27, 2023:

I think the headers in this codebase that include boost should IWYU export the boost include, because it is an implementation detail limited to this single module. Modules including this module shouldn’t be bothered with implementation details of it.

maflcko commented at 3:10 pm on January 15, 2024:

are you still working on this?

maflcko approved

maflcko commented at 10:57 am on May 29, 2023: member

lgtm ACK

DrahtBot removed the label CI failed on May 29, 2023

hebasto force-pushed on May 29, 2023

hebasto commented at 1:00 pm on May 29, 2023: member

Addressed @MarcoFalke’s #27710 (review).

in contrib/devtools/iwyu/bitcoin.core.imp:30 in b2634ad1ed outdated

28+  { include: [ "<boost/test/utils/basic_cstring/basic_cstring.hpp>", public, "<boost/test/unit_test.hpp>", public ] },
29+  { include: [ "<boost/test/utils/lazy_ostream.hpp>", public, "<boost/test/unit_test.hpp>", public ] },
30+
31+  # Boost.Signals2
32+  { symbol: ["boost::function", "private", "<boost/signals2/signal.hpp>", "public" ] },
33+  { symbol: ["boost::optional", "private", "<boost/signals2/optional_last_value.hpp>", "public" ] },

maflcko commented at 10:38 am on May 30, 2023:

Seems an implementation detail of boost/signal that should map to boost/signals2/signal.hpp?

Edit: NVM. This is a public class. https://www.boost.org/doc/libs/1_82_0/doc/html/boost/signals2/optional_last_value.html

hebasto commented at 10:47 am on May 30, 2023:

Looking into the boost/signals2/signal.hpp and boost/signals2/optional_last_value.hpp headers, I’d say that the boost/optional.hpp header is an implementation details of the latter, not former.

maflcko commented at 10:47 am on May 30, 2023: member

Forget my earlier comments. I think this is correct, or at least a good step in the right direction.

lgtm ACK b2634ad1ed884ff7cd32f00cb45075c100902763

hebasto commented at 10:50 am on May 30, 2023: member

Forget my earlier comments.

Including #27710 (review)?

in contrib/devtools/iwyu/bitcoin.core.imp:19 in b2634ad1ed outdated

0@@ -1,7 +1,33 @@
1 # Fixups / upstreamed changes
2 [
3-  { include: [ "<bits/termios-c_lflag.h>", private, "<termios.h>", public ] },
4-  { include: [ "<bits/termios-struct.h>", private, "<termios.h>", public ] },
5-  { include: [ "<bits/termios-tcflow.h>", private, "<termios.h>", public ] },
6   { include: [ "<bits/chrono.h>", private, "<chrono>", public ] },
7+
8+  # Boost.Test

fanquake commented at 2:11 pm on May 30, 2023:

What’s the reason for leaving certain mappings out of this list, i.e #include <boost/operators.hpp>?

I’m guessing it’s because those same includes are recommended for non-test code, and mapping them to Boost Test here would just produce nonsense recommendations for other files? If so, how are you planning on working around that in future? If not, or that isn’t a problem, why not do the mapping now?

hebasto commented at 5:03 pm on May 30, 2023:

What’s the reason for leaving certain mappings out of this list, i.e #include <boost/operators.hpp>?

To keep this PR in reviewable state. For example, a bunch of public Boost headers must be included explicitly and added to a linter:

boost/multi_index/identity.hpp
boost/multi_index/indexed_by.hpp
boost/multi_index/tag.hpp
boost/tuple/tuple.hpp

UPD: see #27783

I’m guessing it’s because those same includes are recommended for non-test code, and mapping them to Boost Test here would just produce nonsense recommendations for other files? If so, how are you planning on working around that in future? If not, or that isn’t a problem, why not do the mapping now?

That’s not a problem, at all. The PR has been updated.

hebasto force-pushed on May 30, 2023

hebasto commented at 5:05 pm on May 30, 2023: member

Addressed @fanquake’s #27710 (review).

hebasto force-pushed on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

fanquake commented at 9:04 am on May 31, 2023: member

Addressed @fanquake’s #27710 (review).

I don’t see how this is addressed, looks like it’s now just producing include suggestions that are incorrect/we don’t want for the test code.

hebasto commented at 10:02 am on May 31, 2023: member

@fanquake

… looks like it’s now just producing include suggestions that are incorrect/we don’t want for the test code.

Can you point out these suggestions please? Asking because I’ve just skimmed suggestions for test/*_tests.cpp files again and I can see none of them in comparison to the master branch.

DrahtBot removed the label CI failed on May 31, 2023

hebasto force-pushed on Jun 13, 2023

DrahtBot added the label CI failed on Jun 13, 2023

hebasto force-pushed on Jun 14, 2023

DrahtBot removed the label CI failed on Jun 14, 2023

achow101 requested review from maflcko on Sep 20, 2023

maflcko commented at 9:33 am on September 21, 2023: member

Needs rebase for fresh CI?

hebasto force-pushed on Sep 21, 2023

hebasto commented at 11:25 am on September 21, 2023: member

Needs rebase for fresh CI?

Done.

DrahtBot added the label Needs rebase on Nov 30, 2023

ci, iwyu: Use Boost and Qt upstream mapping files d435ad83f0

ci, iwyu: Add Boost.MultiIndex specific mappings c8912e45eb

ci, iwyu: Add Boost.Signals2 specific mappings eeed756b1e

ci, iwyu: Add Boost.Test specific mappings b5c6df6d10

hebasto force-pushed on Jan 5, 2024

hebasto commented at 6:07 pm on January 5, 2024: member

Rebased.

DrahtBot removed the label Needs rebase on Jan 5, 2024

fanquake referenced this in commit 014f52550b on Jan 11, 2024

DrahtBot added the label CI failed on Jan 13, 2024

hebasto commented at 2:33 pm on January 16, 2024: member

#27710 (review)

are you still working on this?

I don’t think I will work on this PR in the nearest future.

hebasto closed this on Jan 16, 2024

hebasto added the label Up for grabs on Jan 16, 2024

ci, iwyu: Update mappings #27710

Code Coverage

Reviews

Conflicts