Skip to content

Apply IWYU to code#6253

Closed
steven-johnson wants to merge 11 commits intomasterfrom
srj/iwyu-6
Closed

Apply IWYU to code#6253
steven-johnson wants to merge 11 commits intomasterfrom
srj/iwyu-6

Conversation

@steven-johnson
Copy link
Copy Markdown
Contributor

@steven-johnson steven-johnson commented Sep 20, 2021

Follow-on PR to #6251 that actually applies IWYU. (Note that #6251 must land before this.)

@steven-johnson steven-johnson changed the title wip Apply IWYU to code Sep 21, 2021
@steven-johnson steven-johnson marked this pull request as ready for review September 21, 2021 16:34
@steven-johnson steven-johnson requested review from abadams and removed request for alexreinking September 21, 2021 16:34
@abadams
Copy link
Copy Markdown
Member

abadams commented Sep 21, 2021

I guess I'm not really convinced that transitive includes are a bad thing. This just seems like it adds lots of lines of code (e.g. including Error.h everywhere).

Definitely worth removing unused includes though. Can we set it up to just detect that?

@steven-johnson
Copy link
Copy Markdown
Contributor Author

I guess I'm not really convinced that transitive includes are a bad thing. This just seems like it adds lots of lines of code (e.g. including Error.h everywhere).

At a very general level I disagree; having each file explicitly include what it uses is IMHO a best practice and makes the code more resilient. But I really don't feel strongly about it. I'll leave this sitting open for a while to see if there are other people with strong feelings in any direction.

Definitely worth removing unused includes though. Can we set it up to just detect that?

Eh, maybe, I'll circle back at some point and see.

@steven-johnson
Copy link
Copy Markdown
Contributor Author

At this point I don't think this is worth any more thought -- I'm gonna close this out (it can be regenerated with modest effort using #6251 if we want)

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.

2 participants