This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Preserve include blocks in ObjC Google style
ClosedPublic

Authored by krasimir on Apr 4 2019, 6:15 AM.

Details

Summary

r357567 started to regroup include block for Google style; it was meant to apply
only for C++. This patch reverts this for ObjC.

Diff Detail

Event Timeline

krasimir created this revision.Apr 4 2019, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 6:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ioeric added inline comments.Apr 4 2019, 6:20 AM
lib/Format/Format.cpp
787

maybe we should also only use regroup for cpp? regroup is only supported in sortCppIncludes after all.

krasimir marked 2 inline comments as done.Apr 4 2019, 6:33 AM
krasimir added inline comments.
lib/Format/Format.cpp
787

Yeah, but they are just not used for unrelated languages. This argument applies to the other fields in the IncludeStyle group above this. I'd lean on keeping this as-is.

ioeric accepted this revision.Apr 4 2019, 6:34 AM
This revision is now accepted and ready to land.Apr 4 2019, 6:34 AM
thakis added a subscriber: thakis.Apr 4 2019, 6:42 AM

Why would we want this to be different in Obj-C and C++?

krasimir marked an inline comment as done.Apr 4 2019, 6:46 AM

Why would we want this to be different in Obj-C and C++?

Only the C++ Style Guide had been updated.
Practically, clang-format would need to be updated to recognize ObjC-specific code patterns better before being able to apply regrouping to ObjC code in the wild.

I'd expect that sometime in the future the ObjC-style catches up.

krasimir updated this revision to Diff 193699.Apr 4 2019, 7:01 AM
  • Add a note about intent
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 7:02 AM
thakis added a comment.Apr 4 2019, 7:14 AM

Please don't land changes with open discussion.

Objective-C++ basically is C++ code. I don't think it makes sense to make a distinction here.

Looks good to me.

cfe/trunk/lib/Format/Format.cpp
881 ↗(On Diff #193700)

nit: "headers"