ci, iwyu: Update mappings #27710
ci, iwyu: Update mappings #27710
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
Rebased on top of the merged #27707. |
|
CI failure seems unrelated. |
| { include: [ "<boost/test/unit_test_log.hpp>", public, "<boost/test/unit_test.hpp>", public ] }, | ||
| { include: [ "<boost/test/unit_test_suite.hpp>", public, "<boost/test/unit_test.hpp>", public ] }, | ||
| { include: [ "<boost/test/utils/basic_cstring/basic_cstring.hpp>", public, "<boost/test/unit_test.hpp>", public ] }, | ||
| { include: [ "<boost/test/utils/lazy_ostream.hpp>", public, "<boost/test/unit_test.hpp>", public ] }, |
There was a problem hiding this comment.
Would it make sense to upstream those?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
I've skipped non-test Boost stuff intentionally to avoid potential conflicts with other (more important) PRs.
There was a problem hiding this comment.
Same for
#include <boost/multi_index/hashed_index.hpp> // for hashed_index_iteratoretc ... ?
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?
There was a problem hiding this comment.
Ah, I think that boost is an implementation detail, so it should be IWYU export ed from the header that exports the boost include.
There was a problem hiding this comment.
Do you mean headers in the boost codebase, or in ours?
There was a problem hiding this comment.
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.
1279c99 to
b2634ad
Compare
|
Addressed @MarcoFalke's #27710 (comment). |
|
Forget my earlier comments. I think this is correct, or at least a good step in the right direction. lgtm ACK b2634ad |
Including #27710 (comment)? |
|
Addressed @fanquake's #27710 (comment). |
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. |
Can you point out these suggestions please? Asking because I've just skimmed suggestions for |
|
Needs rebase for fresh CI? |
0181123 to
2dc5025
Compare
Done. |
|
Rebased. |
a395218 ci, iwyu: Drop backported mappings (Hennadii Stepanov) Pull request description: See include-what-you-use/include-what-you-use#1026. Split from #27710 as a non-controversial change. ACKs for top commit: fanquake: ACK a395218 Tree-SHA512: ae7955a99396ab4f62ab7c989dba59c26448837f0ba4436bf3fddebe4099b5d3c03492e22a8497104c6afcceede1bb1b81f9d71c7c7e43692e6d70dcdfc11e7c
I don't think I will work on this PR in the nearest future. |
This PR reduces false warnings, especially for unit tests.
From the Boost.Test's point of view, the
boost/test/unit_test.hppheader: