Page MenuHomePhabricator

[clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.
ClosedPublic

Authored by chandlerc on Jun 6 2017, 3:56 AM.

Details

Summary

This really is a collection of improvements to the rules for LLVM
include sorting:

  • We have gmock headers now, so it adds support for those to one of the categories.
  • LLVM does use 'FooTest.cpp' files to test 'Foo.h' so it adds that suffix for finding a main header.
  • At times the test file's case may not match the header file's case, so adds support for case-insensitive regex matching of these things.

With this set of changes, I can't spot any misbehaviors when re-sorting
all of LLVM's unittest '#include' lines.

Note that I don't know the first thing about testing clang-format, so please
feel free to tell me what all I need to be doing here.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Jun 6 2017, 3:56 AM
ioeric accepted this revision.Jun 6 2017, 4:28 AM

Lgtm.

lib/Format/Format.cpp
1406 ↗(On Diff #101535)

Also add tests for these non-main categories?

This revision is now accepted and ready to land.Jun 6 2017, 4:28 AM
chandlerc updated this revision to Diff 101544.Jun 6 2017, 4:53 AM

Add test coverage for the case-insensitive category logic.

chandlerc marked an inline comment as done.Jun 6 2017, 4:53 AM

Added more tests, PTAL?

ioeric added inline comments.Jun 6 2017, 5:10 AM
unittests/Format/SortIncludesTest.cpp
269 ↗(On Diff #101544)

I think this TEST is only for main header sorting. Could you split tests for other categories into a separate TEST?

chandlerc updated this revision to Diff 103885.Jun 25 2017, 6:53 PM
chandlerc marked an inline comment as done.

Update based on review comments.

chandlerc requested review of this revision.Jun 25 2017, 6:53 PM
chandlerc edited edge metadata.

Tried to address the test comment -- let me know if something else was intended.

djasper added inline comments.
include/clang/Format/Format.h
993 ↗(On Diff #103885)

Do we really need a flag here? Shouldn't we just always do this?

chandlerc added inline comments.Jun 27 2017, 3:07 PM
include/clang/Format/Format.h
993 ↗(On Diff #103885)

I have no opinion one way or the other. I opted to not change behavior for existing styles purely in the spirit of making as targeted of a change as I could..

Feel free to tell me how you want this to work / be tested.

Just make clang-format always do this. I don't think anyone is relying on the current behavior.

Update based an Daniel's feedback.

Just make clang-format always do this. I don't think anyone is relying on the current behavior.

Done, PTAL.

djasper accepted this revision.Jun 29 2017, 4:56 AM

Looks good. Thank you.

This revision is now accepted and ready to land.Jun 29 2017, 4:56 AM
This revision was automatically updated to reflect the committed changes.

Just make clang-format always do this. I don't think anyone is relying on the current behavior.

Well, someone did rely on case sensitive match. I thought case insensitive by default was just a Windows thing. Not anymore.

- Regex:           '^"I[A-Z]+[a-z].*\.h"'
  Priority:        1

This rule became too greedy on latest revision. Now not only interfaces are prioritized but also all headers that start with [Ii].