Proposing misc-include-cleaner check

[Takuto Ikuta]

Hi,

This is a proposal to enable misc-include-cleaner check.
See this doc for analysis.

Feedbacks are welcome.

Thanks,
Takuto

[Peter Kasting]

It looks like this fires a ton, and it has some sharp edges. At the very least, we'd need easily-findable docs on what to do when e.g. the check suggests the wrong header (i.e. how people can correct the check for everyone). It should also probably have recommendations at least for STL headers (not knowing where std::move comes from is unfortunate).

Once we've picked the low-hanging fruit, is there a way we can auto-fix most instances in a subdirectory, and then enable the check there? I fear people getting annoyed and ignoring the check if it fires too much, and about pre existing code. 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALNjmMqSGpNhnnNqWJiTHSP96FkpaLxyyAhSNCHfNAKDmNsLGA%40mail.gmail.com.

[Takuto Ikuta]

Thank you for the feedback.

Having easily findable docs sounds like a good idea. For that, I think we can have something like docs/include_what_you_use.md and change recipe to show the link to the doc for misc-include-cleaner warnings.

We can also hide warnings if that somehow doesn't have replacement suggestions while filing issues to upstream.

For the auto-fix, I can run clang-tidy to the entire codebase and send CLs as LSC. Although I think we still need to fix some cases manually like build target mismatch between include and usages and add IWYU pragmas to many places to prevent unintentional include modifications.

Also I think warnings from clang-tidy are not necessary to fix, and comments from tricium are shown only in the modified area for now. So I guess enabling this may not produce many annoying warnings to existing CLs.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFD2why1Arp6M762C9bcNeHBtQp%2BAT-3WjPTrQxW%3Dq0xkA%40mail.gmail.com.

[Kaylee Lubick]

What would the plan look like for rolling this out incrementally? Skia has been rolling out enforcement of IWYU slowly by having an opt-in list of files and directories and building that up over time. This has helped minimize the scope of changes, especially when a widely-used header is fixed up (breaking other files that were transitively depending on some removed headers, for example). I'm worried that trying to fix the whole code base and then break that up into CLs will run into similar issues and be tough to land.

IWYU also lets you specify which symbols come from where or not to use private header files via a mapping file. If clang-tidy doesn't let us do that easily, that seems like it will cause lots of headaches down the road.

Kaylee

[John Stiles]

+1 to Kaylee's approach. As things are cleaned up, it will cause sometimes surprisingly broad ripple effects. Opting in individual directories at a time makes a lot of sense; trying to do all of Chromium in one go is likely to be frustrating.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/284c0d2f-61bc-4623-94a8-e45f183be86en%40chromium.org.

[Peter Kasting]

On Thursday, April 25, 2024 at 12:35:05 AM UTC-7 Takuto Ikuta wrote:

Also I think warnings from clang-tidy are not necessary to fix, and comments from tricium are shown only in the modified area for now. So I guess enabling this may not produce many annoying warnings to existing CLs.

There's also clangd include-cleaner warnings, which ideally we'd want (since they surface these issues earlier and in an automatically-fixable way, for people who use clangd-integrated editors), but right now we disable because they fire everywhere: https://source.chromium.org/chromium/chromium/src/+/main:.clangd

With other clang-tidy checks, we've preferred to enable them only if they have few remaining unfixed instances in the codebase. I think having a plan for how to incrementally fix-and-roll-out both the clang-tidy and the clangd versions of this is the right way to go, compared to trying to enable codebase-wide with or without LSC-style mass fixes. IMO this would be appropriate for a code health rotation project if you have a trustworthy set of steps that can fix up a local area.

PK

[Jean-Philippe Gravel]

The clangd and clang-tidy checks can be enabled on a per-folder basis, which could allow for a progressive rollout. See https://crrev.com/c/5472985 for instance.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/5dd832e3-78dc-4b0a-8d6a-eb187c074ff9n%40chromium.org.

[Takuto Ikuta]

Thank you for your responses.

If our preference is enabling this check incrementally, I'll write a small instruction for this and propose to code health rotation then.

Will update this thread once I prepare the CLs for that.

Thanks,

Takuto

[Takuto Ikuta]

I made https://crrev.com/c/5514394 for the instructions.