This is an archive of the discontinued LLVM Phabricator instance.

[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].

I just came across this case-insensitive behavior when trying to regroup include blocks and it was surprising. I'm trying to put system headers after other angle-bracket includes (like for Qt), and I was trying:

- Regex:           '^<[a-z_]*>'
  Priority:        5
- Regex:           '^<[A-Za-z_]*>'
  Priority:        4

Unfortunately, #include <QWidget> matches the first regex and so Qt headers get sorted in with system headers. Would it be possible to add the option to do case-sensitive matching after all? Should I open a new issue?

For anyone else who wants case-sensitive matching, I just did discover that (?-i) seems to do the trick. I thought it wouldn't because the docs mention extended regex, but I guess it's more extended than I thought! Using '(?-i)^<[a-z_]*>' does the trick for me.

Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2019, 8:59 AM

It appears I was mistaken - the (?-i) caused the regex match to fail entirely, which made the system header move all the way to the end. It didn't actually solve my problem.