This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix bug where -dump-config failed on ObjC header
ClosedPublic

Authored by benhamilton on Jan 22 2018, 2:08 PM.

Details

Summary

clang-format -dump-config path/to/file.h never passed
anything for the Code parameter to clang::format::getStyle().

This meant the logic to guess Objective-C from the contents
of a .h file never worked, because LibFormat didn't have the
code to work with.

With this fix, we now correctly read in the contents of the
file if possible with -dump-config.

I had to update the lit config for test/Format/ because
the default config ignores .h files.

Test Plan: make -j12 check-clang

Diff Detail

Event Timeline

benhamilton created this revision.Jan 22 2018, 2:08 PM
  • Support AssumeFileName with stdin.
jolesiak requested changes to this revision.Jan 25 2018, 2:37 AM
jolesiak added inline comments.
test/Format/lit.local.cfg
2–3

Where does this list come from?
Shouldn't '.textpb', 'METADATA', '.c', '.cc' be added here?

This revision now requires changes to proceed.Jan 25 2018, 2:37 AM
  • Add more extensions.
benhamilton marked an inline comment as done.Jan 25 2018, 9:30 AM
benhamilton added inline comments.
test/Format/lit.local.cfg
2–3

It's a good question, thanks.

I copied this list from LibFormat's getLanguageByFileName():

https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L2265

I did miss .textpb, added now. I also added .c and .cc since they should default to C++ style.

METADATA isn't a suffix understood by clang-format, so I did not add it.

jolesiak accepted this revision.Jan 26 2018, 12:31 AM
This revision is now accepted and ready to land.Jan 26 2018, 12:31 AM
krasimir requested changes to this revision.Jan 26 2018, 1:55 AM
krasimir added inline comments.
test/Format/lit.local.cfg
2

Why is this needed?

This revision now requires changes to proceed.Jan 26 2018, 1:55 AM
benhamilton requested review of this revision.Jan 26 2018, 1:26 PM
benhamilton marked 2 inline comments as done.
benhamilton added inline comments.
test/Format/lit.local.cfg
2

'.h' is not in the list of file suffixes which lit is configured to look for:

https://github.com/llvm-mirror/clang/blob/master/test/lit.cfg.py#L28

So, we need to provide our own list of file suffixes here.

Other tests do the same:

https://github.com/llvm-mirror/clang/search?utf8=%E2%9C%93&q=%22config.suffixes%22&type=

so I think it's OK to do so here as well.

krasimir accepted this revision.Jan 29 2018, 3:08 AM
krasimir added inline comments.
test/Format/lit.local.cfg
2

Thank you!

This revision is now accepted and ready to land.Jan 29 2018, 3:08 AM
This revision was automatically updated to reflect the committed changes.
benhamilton marked an inline comment as done.