Page MenuHomePhabricator

Don't allow llvm-include-order to intermingle includes from different files.
ClosedPublic

Authored by zturner on Aug 11 2016, 4:19 PM.

Details

Summary

See llvm.org/pr28943 for some context.

I'm not sure if this is the right fix, but it seems to pass the test suite for me.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 67771.Aug 11 2016, 4:19 PM
zturner retitled this revision from to Don't allow llvm-include-order to intermingle includes from different files..
zturner updated this object.
zturner added reviewers: alexfh, djasper.
zturner added a subscriber: cfe-commits.
zturner updated this revision to Diff 67772.Aug 11 2016, 4:32 PM

Fixed the test for this. Confirmed that it fails before the patch and succeeds after the patch.

zturner updated this revision to Diff 67777.Aug 11 2016, 5:09 PM

Seems like dropping all non-main-file include directives is wrong because then it won't be able to detect errors in "non user code". Seems like the real fix is to bucket the include directives by file in which they were declared in, and then process each bucket in order. This way we process all include directives, including those in non-user code, but block analysis is done at a per-file level.

djasper edited edge metadata.Aug 12 2016, 12:36 AM

I think we should entirely drop this implementation of the check and let it just check #includes with clang-format. clang-format's implementation isn't a strict superset, e.g. won't sort between multiple blocks, but that's intentional for now.

alexfh accepted this revision.Aug 12 2016, 8:20 AM
alexfh edited edge metadata.

The patch LG.

I think we should entirely drop this implementation of the check and let it just check #includes with clang-format. clang-format's implementation isn't a strict superset, e.g. won't sort between multiple blocks, but that's intentional for now.

Since sorting across blocks is frequently needed to make include order consistent with the LLVM coding standards, it makes sense to keep this check until clang-format is able to sort across blocks. Especially, since Zachary is specifically interested in this functionality.

This revision is now accepted and ready to land.Aug 12 2016, 8:20 AM

I think we got confused. We once had tried to write an experimental separate check to comply with Google's style guide. If you want to fiddle around with that, contact me, I can send you pointers. But as I mentioned we moved away from that. And I think it makes more sense to re-create the sort-across-blocks functionality in clang-format and not in clang-tidy.

I think we got confused. We once had tried to write an experimental separate check to comply with Google's style guide. If you want to fiddle around with that, contact me, I can send you pointers. But as I mentioned we moved away from that. And I think it makes more sense to re-create the sort-across-blocks functionality in clang-format and not in clang-tidy.

Yep, we definitely got confused. That experimental check actually implemented cross-block sorting, but this caused a bunch of issues. Which makes me think that proper implementation of cross-block include sorting is challenging be it in clang-format or clang-tidy. Clang-format probably makes it even more complex, since a higher safety of transformations is expected from it.

This revision was automatically updated to reflect the committed changes.