Page MenuHomePhabricator

Remove unnecessary includes from lld/ELF
Needs ReviewPublic

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



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!