This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Always indent wrapped Objective-C selector names
ClosedPublic

Authored by benhamilton on Mar 28 2018, 3:46 PM.

Details

Summary

Currently, indentation of Objective-C method names which are wrapped
onto the next line due to a long return type is controlled by the
style option IndentWrappedFunctionNames.

This diff changes the behavior so we always indent wrapped Objective-C
selector names.

NOTE: I partially reverted https://github.com/llvm-mirror/clang/commit/6159c0fbd1876c7f5f984b4830c664cc78f16e2e / rL242484, as it was causing wrapped selectors to be double-indented. Its tests in FormatTestObjC.cpp still pass.

Test Plan: Tests updated. Ran tests with:

% make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

Diff Detail

Repository
rC Clang

Event Timeline

benhamilton created this revision.Mar 28 2018, 3:46 PM

What do you think of the name IndentWrappedObjCMethodSignatures as an alternative to IndentWrappedObjCMethodNames? In Objective-C the method name might be considered the selector whereas I believe that this option pertains to formatting of the method signature?

What do you think of the name IndentWrappedObjCMethodSignatures as an alternative to IndentWrappedObjCMethodNames? In Objective-C the method name might be considered the selector whereas I believe that this option pertains to formatting of the method signature?

This is a good suggestion, but since this is a specialization of IndentWrappedFunctionNames, I figured keeping the name of the new option similar to that would be best.

stephanemoore added inline comments.Mar 28 2018, 4:15 PM
include/clang/Format/Format.h
1154–1163 ↗(On Diff #140155)

Do we explicitly want these to be generic with the intent to later reuse the enum for C++ method wrapping style?

Do we have a public style guide that explicitly says to do this?
That's a requirement for new style options as per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.

Also, are we sure that somebody wants the other behavior? Does that make sense for ObjC? I'd like to avoid options that nobody actually sets to a different value.

benhamilton marked an inline comment as done.Mar 29 2018, 8:33 AM
benhamilton added inline comments.
include/clang/Format/Format.h
1154–1163 ↗(On Diff #140155)

I figured languages like Java and JS might want to customize this.

benhamilton marked an inline comment as done.Mar 29 2018, 8:38 AM

Do we have a public style guide that explicitly says to do this?

Good question! This was the result of internal discussion with the Google Objective-C style guide maintainers based on analysis of ObjC code at Google.

I took a look, and the C++ style guide doesn't seem to explicitly say to align wrapped function names on column 0 (although clang-format does that):

http://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
http://google.github.io/styleguide/cppguide.html#Function_Calls

I will confirm with the ObjC style guide maintainers whether they want to explicitly list the desired ObjC behavior in the style guide.

Well, I disagree. It says: "If you break after the return type of a function declaration or definition, do not indent."

Well, I disagree. It says: "If you break after the return type of a function declaration or definition, do not indent."

Great, thanks for clarifying!

Update:
I am proposing to update the Google Objective-C style guide to explicitly describe the desired indentation behavior.

include/clang/Format/Format.h
1154–1163 ↗(On Diff #140155)

Gotcha; sounds reasonable to me.

I am proposing to update the Google Objective-C style guide to explicitly describe the desired indentation behavior.

+1

I still really believe that these config options do no make sense and are actively confusing.

I see two options:

  • We leave this as is
  • We fix this right

The right fix here is (IMO) that a style already is per language and thus already has a member specifying the language. What we have done in the scope of formatting the contents of raw string literals is that you can, in the same configuration file/setting, have different styles based on the language being formatted. Thus, if we encounter a text-formatted proto inside a C++ raw string literal, we switch to the style flags for proto rather than using those for C++. You have these different options in the same style configuration file, in a different section per language.

So, if you look at a config file, you could see how a user sets the existing IndentWrappedFunctionNames to true for ObjC and to false for C++. Now IMO, that should mean that ObjC function names are indented and C++ functions are not, even if the language of the *file* is ObjC. It doesn't require us to repeat these options for each language in the style for each language.

Getting this right will require some refactoring of how a style is passed around and used, but I think it'd be the right thing to do.

Look at it the other way. If we go forward with this patch you can have style configuration files saying:


BasedOnStyle: LLVM
---
Language: Cpp
IndentWrappedObjCMethodNames: Never
IndentWrappedFunctionNames: true
---
Language: ObjC
IndentWrappedObjCMethodNames: Always
IndentWrappedFunctionNames: false
---

I find it very hard to explain what that even means.

a user sets the existing IndentWrappedFunctionNames to true for ObjC and to false for C++.
Now IMO, that should mean that ObjC function names are indented and C++ functions are not, even if the language of the *file* is ObjC.
Getting this right will require some refactoring of how a style is passed around and used, but I think it'd be the right thing to do.

This is an interesting proposal, but adding infrastructure to pass around and manage per-language FormatStyle objects to formatter logic seems like a large change to me, and a lot of work to avoid adding a new configuration option.

Without changing the architecture to pass around per-language FormatStyles, we could also unconditionally indent wrapped Objective-C methods in all styles (ignoring IndentWrappedFunctionNames) and make IndentWrappedFunctionNames only apply to C++ functions and methods.

I just did a survey of open-source Objective-C code in use at Google, and it's over 5:1 in favor of indenting wrapped Objective-C methods.

What do you think of that option?

I'd go to great lengths to avoid adding new config options and so I don't think this would be a bad trade-off to make.

Also, note that you might not actually have to change much. A FormatStyle already contains a reference to the FormatStyleSet it was created from and you can get the FormatStyle for a different language through that. It might just be a bit hacky and not easy to understand as is, but maybe there are easy abstractions that we could build around it.

At any rate, your proposal (always indenting for ObjC code) is also fine by me. Do you think that survey Google-style ObjC code is enough? What does Xcode do by default?

A FormatStyle already contains a reference to the FormatStyleSet it was created from and you can get the FormatStyle for a different language through that. It might just be a bit hacky and not easy to understand as is, but maybe there are easy abstractions that we could build around it.

Hmm, I didn't know that. That would make a short-term hack easier.

At any rate, your proposal (always indenting for ObjC code) is also fine by me. Do you think that survey Google-style ObjC code is enough? What does Xcode do by default?

Apple SDKs have no line length limit and don't wrap code as far as I can tell, so they don't hit this issue.

Xcode neither indents nor aligns colons by default. It does no code formatting at all as far as I can tell.

I guess the question is, are there other big clients of Objective-C formatting via clang-format besides Google? I looked at the existing public coding styles, and none of them appear to talk about this issue.

Actually, a slight correction. By default, Xcode has a "Line wrapping" setting under Preferences -> Text editing which says:

[ X ] Wrap lines to editor width
     Indent wrapped lines by:  [    4 ] Spaces

which suggests people using Xcode may expect indentation on wrapped lines by default.

It also has a "Syntax-aware indenting" feature including "Automatic indent for... :", but the colon indentation only seems to apply for method invocations, not method signatures.

By default, Xcode has a "Line wrapping" setting

Ah, that appears to just be a visual thing. Xcode doesn't actually insert newlines into the source code.

Ok, you know the ObjC community much better than I do. If you think that adding the indentation for ObjC irrespective of the option works for most people, I am happy to go with it.

benhamilton retitled this revision from [clang-format] New style option IndentWrappedObjCMethodNames to [clang-format] Always indent wrapped Objective-C selector names.Apr 10 2018, 9:46 AM
benhamilton edited the summary of this revision. (Show Details)
  • Unconditionally indent wrapped ObjC selector names.
  • Partially revert r242484, as it was causing double-indenting (the tests still pass with it reverted, so I suspect it's no longer necessary).
benhamilton added a comment.EditedApr 10 2018, 9:49 AM

Ok, you know the ObjC community much better than I do. If you think that adding the indentation for ObjC irrespective of the option works for most people, I am happy to go with it.

OK, I went ahead and implemented unconditional indenting of wrapped ObjC selectors and removed the configuration setting.

I had to partially revert rL242484; with it, we were getting 8-column indenting, and without it, its tests still pass, so I suspect it's no longer necessary.

djasper accepted this revision.Apr 12 2018, 5:33 AM
djasper added inline comments.
unittests/Format/FormatTest.cpp
7681

maybe: .. and always indents. ?

This revision is now accepted and ready to land.Apr 12 2018, 5:33 AM
benhamilton marked an inline comment as done.

Update comment.

benhamilton added inline comments.Apr 12 2018, 8:05 AM
unittests/Format/FormatTest.cpp
7681

Sure, done.

This revision was automatically updated to reflect the committed changes.