[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
djasper added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton added a comment. > 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton added a comment. > 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
djasper added a comment. 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? Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton added a comment. > 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 `FormatStyle`s, 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? Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
djasper added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
jolesiak added a comment. > I am proposing to update the Google Objective-C style guide to explicitly > describe the desired indentation behavior. +1 Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
stephanemoore added a comment.
Update:
I am proposing to update the Google Objective-C style guide to explicitly
describe the desired indentation behavior.
Comment at: include/clang/Format/Format.h:1154-1163
+ /// \brief The style of indenting long function or method names wrapped
+ /// onto the next line.
+ enum IndentWrappedMethodStyle {
+/// Automatically determine indenting style.
+IWM_Auto,
+/// Always indent wrapped method names.
+IWM_Always,
benhamilton wrote:
> stephanemoore wrote:
> > Do we explicitly want these to be generic with the intent to later reuse
> > the enum for C++ method wrapping style?
> I figured languages like Java and JS might want to customize this.
Gotcha; sounds reasonable to me.
Repository:
rC Clang
https://reviews.llvm.org/D45004
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton added a comment. > 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! Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
djasper added a comment. Well, I disagree. It says: "If you break after the return type of a function declaration or definition, do not indent." Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton marked an inline comment as done. benhamilton added a comment. > 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton marked an inline comment as done.
benhamilton added inline comments.
Comment at: include/clang/Format/Format.h:1154-1163
+ /// \brief The style of indenting long function or method names wrapped
+ /// onto the next line.
+ enum IndentWrappedMethodStyle {
+/// Automatically determine indenting style.
+IWM_Auto,
+/// Always indent wrapped method names.
+IWM_Always,
stephanemoore wrote:
> Do we explicitly want these to be generic with the intent to later reuse the
> enum for C++ method wrapping style?
I figured languages like Java and JS might want to customize this.
Repository:
rC Clang
https://reviews.llvm.org/D45004
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
djasper added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
stephanemoore added inline comments.
Comment at: include/clang/Format/Format.h:1154-1163
+ /// \brief The style of indenting long function or method names wrapped
+ /// onto the next line.
+ enum IndentWrappedMethodStyle {
+/// Automatically determine indenting style.
+IWM_Auto,
+/// Always indent wrapped method names.
+IWM_Always,
Do we explicitly want these to be generic with the intent to later reuse the
enum for C++ method wrapping style?
Repository:
rC Clang
https://reviews.llvm.org/D45004
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton added a comment. > 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. Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
stephanemoore added a comment. 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? Repository: rC Clang https://reviews.llvm.org/D45004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton updated this revision to Diff 140155.
benhamilton added a comment.
Fix typo
Repository:
rC Clang
https://reviews.llvm.org/D45004
Files:
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
unittests/Format/FormatTestObjC.cpp
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -539,6 +539,36 @@
":(int)a\n"
" aaa:(int)a;\n");
+ Style.IndentWrappedObjCMethodNames = FormatStyle::IWM_Always;
+ verifyFormat("- (a)\n"
+ ";\n");
+ verifyFormat("- (a)\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ " aaa:(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ " aaa:(int)a;\n");
+
+ Style.IndentWrappedObjCMethodNames = FormatStyle::IWM_Never;
+ verifyFormat("- (a)\n"
+ ";\n");
+ verifyFormat("- (a)\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ " aaa:(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ " aaa:(int)a;\n");
+
// Continuation indent width should win over aligning colons if the function
// name is long.
Style = getGoogleStyle(FormatStyle::LK_ObjC);
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -243,6 +243,16 @@
}
};
+template <>
+struct ScalarEnumerationTraits {
+ static void enumeration(IO &IO,
+ FormatStyle::IndentWrappedMethodStyle &Value) {
+IO.enumCase(Value, "Auto", FormatStyle::IWM_Auto);
+IO.enumCase(Value, "Always", FormatStyle::IWM_Always);
+IO.enumCase(Value, "Never", FormatStyle::IWM_Never);
+ }
+};
+
template <> struct MappingTraits {
static void mapping(IO &IO, FormatStyle &Style) {
// When reading, read the language first, we need it for getPredefinedStyle.
@@ -378,6 +388,8 @@
IO.mapOptional("IndentWidth", Style.IndentWidth);
IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentWrappedObjCMethodNames",
+ Style.IndentWrappedObjCMethodNames);
IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
@@ -645,6 +657,7 @@
LLVMStyle.IndentCaseLabels = false;
LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
LLVMStyle.IndentWrappedFunctionNames = false;
+ LLVMStyle.IndentWrappedObjCMethodNames = FormatStyle::IWM_Auto;
LLVMStyle.IndentWidth = 2;
LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
LLVMStyle.JavaScriptWrapImports = true;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -26,6 +26,19 @@
namespace clang {
namespace format {
+// Returns true if a TT_SelectorName should be indented when wrapped,
+// false otherwise.
+static bool shouldIndentWrappedSelectorName(const FormatStyle &Style) {
+ // TT_SelectorName is used across multiple languages; we only want
+ // Style.IndentWrappedObjCMethodNames to apply to ObjC.
+ if (Style.Language == FormatStyle::LK_ObjC)
+return Style.IndentWrappedObjCMethodNames == FormatStyle::IWM_Always ||
+ (Style.IndentWrappedObjCMethodNames == FormatStyle::IWM_Auto &&
+Style.IndentWrappedFunctionNames);
+ else
+return Style.IndentWrappedFunctionNames;
+}
+
// Returns the length of everything up to the first possible line break after
// the ), ], } or > matching \c Tok.
static unsigned getLengthToMatchingParen(const FormatToken &Tok) {
@@ -698,7 +711,7 @@
State.S
[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames
benhamilton created this revision.
benhamilton added reviewers: djasper, jolesiak.
Herald added subscribers: cfe-commits, klimek.
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`.
For the Google style, we'd like to indent Objective-C method names
when wrapped onto the next line, but *not* indent non-Objective-C
functions when wrapped onto the next line.
This diff adds a new style option, `IndentWrappedObjCMethodNames`,
with three options:
1. Auto: Keep current behavior (indent ObjC methods according to
`IndentWrappedFunctionNames`)
2. Always: Always indent wrapped ObjC methods
3. Never: Never indent wrapped ObjC methods
In a separate diff, I'll update the Google style to set
`IndentWrappedFunctionNames` to `Always`.
Test Plan: Tests updated. Ran tests with:
% make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests
Repository:
rC Clang
https://reviews.llvm.org/D45004
Files:
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
unittests/Format/FormatTestObjC.cpp
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -539,6 +539,36 @@
":(int)a\n"
" aaa:(int)a;\n");
+ Style.IndentWrappedObjCMethodNames = FormatStyle::IWM_Always;
+ verifyFormat("- (a)\n"
+ ";\n");
+ verifyFormat("- (a)\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ " aaa:(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ " aaa:(int)a;\n");
+
+ Style.IndentWrappedObjCMethodNames = FormatStyle::IWM_Never;
+ verifyFormat("- (a)\n"
+ ";\n");
+ verifyFormat("- (a)\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ " aaa:(int)a\n"
+ ":(int)a;\n");
+ verifyFormat("- (a)\n"
+ ":(int)a\n"
+ " aaa:(int)a;\n");
+
// Continuation indent width should win over aligning colons if the function
// name is long.
Style = getGoogleStyle(FormatStyle::LK_ObjC);
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -243,6 +243,16 @@
}
};
+template <>
+struct ScalarEnumerationTraits {
+ static void enumeration(IO &IO,
+ FormatStyle::IndentWrappedMethodStyle &Value) {
+IO.enumCase(Value, "Auto", FormatStyle::IWM_Auto);
+IO.enumCase(Value, "Always", FormatStyle::IWM_Always);
+IO.enumCase(Value, "Never", FormatStyle::IWM_Never);
+ }
+};
+
template <> struct MappingTraits {
static void mapping(IO &IO, FormatStyle &Style) {
// When reading, read the language first, we need it for getPredefinedStyle.
@@ -378,6 +388,8 @@
IO.mapOptional("IndentWidth", Style.IndentWidth);
IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentWrappedObjCMethodNames",
+ Style.IndentWrappedObjCMethodNames);
IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
@@ -645,6 +657,7 @@
LLVMStyle.IndentCaseLabels = false;
LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
LLVMStyle.IndentWrappedFunctionNames = false;
+ LLVMStyle.IndentWrappedObjCMethodNames = FormatStyle::IWM_Auto;
LLVMStyle.IndentWidth = 2;
LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
LLVMStyle.JavaScriptWrapImports = true;
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndent
