Page MenuHomePhabricator

Remove unnecessary includes from lld/ELF
Needs ReviewPublic

Authored by yamaguchi on Mar 15 2018, 1:42 AM.

Details

Summary

These includes are already included in somewhere above the include tree, so we don't have to include them here.

Diff Detail

Event Timeline

yamaguchi created this revision.Mar 15 2018, 1:42 AM
yamaguchi edited the summary of this revision. (Show Details)Mar 15 2018, 1:45 AM
espindola edited reviewers, added: espindola; removed: rafael.Mar 15 2018, 8:31 AM

Generally, relying on indirect inclusion isn't ideal - if the intermediate headers are later refactored & remove the includes, it may break the build of those depending on that indirect inclusion.

Was there a particular motivation for these changes?

Some of these seem to be really unused (mutex in InputSection.cpp).

Ideally we would remove just those. Given that this changes just cpp files, I am somewhat tempted to LGTM this. If we want to avoid indirect includes, how dependable is IWYU these days? Would the bots doing module builds find any problems?

ruiu added a comment.Mar 19 2018, 11:01 AM

When I'm writing code for lld, I'm trying to maintain the include file list so that a .cpp file includes everything that the file uses, as indirect file inclusion feels somewhat fragile and not explicit. Because I'm doing that by hand, I'm pretty sure that we miss a lot of header files, but the spirit is to make the include list more explicit than implicit (and making things explicit and obvious is probably more "lld-ish", I would say). So I'm included to not do this.

Maybe you now have a question as to why we don't enforce it by a tool then? I think if everybody agrees that we should use a tool and always include what you use, that's fine, but I personally don't have enough motivation to propose that. In addition to that, it is in practice hard to ask everyone (including one-time contributors) to use some tool. So status quo is ok to me. Thank you for your patch anyways!