[PATCH] D49580: [clang-format] Adding style option for absolute formatting
acoomans abandoned this revision. acoomans added a comment. Changed approach based on feedback. New changeset: https://reviews.llvm.org/D50535 https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
jolesiak added a comment. Thanks! https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D49580: [clang-format] Adding style option for absolute formatting
Thanks for the review. I’ll update when I’m back from vacation Arnaud > On Jul 30, 2018, at 6:09 AM, Jacek Olesiak via Phabricator > wrote: > > jolesiak added a comment. > > In https://reviews.llvm.org/D49580#1179924, @djasper wrote: > >> Ok, so IIUC, understanding that @end effective ends a section much like "}" >> would address the currently observed problems? > > > I think so. > > > https://reviews.llvm.org/D49580 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
jolesiak added a comment. In https://reviews.llvm.org/D49580#1179924, @djasper wrote: > Ok, so IIUC, understanding that @end effective ends a section much like "}" > would address the currently observed problems? I think so. https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
djasper added a comment. Ok, so IIUC, understanding that @end effective ends a section much like "}" would address the currently observed problems? https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
jolesiak added a comment. First of all, thanks, Arnaud, for looking into this. Let's address the mentioned bug first. Let's assume that we use `IndentReference = Relative`. I think that this particular formatting bug is local (i.e. can be solved while processing a `@protocol` block). Look at this example: if (true) { int a = 42; int b = 42; int c = 42; } int d = 42; When we run `clang-format -lines=4:4` we get: if (true) { int a = 42; int b = 42; int c = 42; } int d = 42; Please note that neither `}` nor `int d = 42` is indented. But this is a different behavior from what we see after running `clang-format -lines=3:3` on: @protocol A @optional // comment - (void)f; @end MACRO the output is: @protocol A @optional // comment - (void)f; @end MACRO Please note that `@end` is indented (has different indentation than `@protocol`; `MACRO` is indented as well). To clarify why this is undesired, `@end` ends `@protocol`, not `@optional`. By the way, `clang-format` doesn't think that `@end` should match `@optional`, check the output of an extended example: @protocol A @optional // comment - (void)f; @end MACRO @end Second `@end` still doesn't match the `@protocol`. I think it's perfectly fine to allow users to have custom indentations if they so choose. In this particular example, though, `clang-format` should stop indenting when it reaches `@end`, giving the following output: @protocol A @optional // comment - (void)f; @end MACRO Moving to the general case, I must say that introducing `IndentReference = Relative/Absolute` is a very interesting approach. I can imagine situations when somebody actually want to use `IndentReference = Absolute`. However, adding an additional option comes at quite big cost and I think that in this case it outweighs the benefits. I think that a very good outcome could be to comment what is happening next to `IndentTracker.adjustToUnmodifiedLine(TheLine);` line. E.g. "We adjust an indentation to match the existing code to give users flexibility (instead of ignoring it). It's their responsibility to provide a correct formatting of lines they didn't initially change if these lines break formatting.". https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
jolesiak added a comment. Sorry for the delay, I'll comment on that tomorrow. https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
acoomans added a comment. I don't know; I just picked a random bug from the Bugzilla to get myself familiarized with the LLVM codebase :) @jolesiak? https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
djasper added a comment. In my opinion, this only addresses one edge case where clang-format -lines output is not identical with a full reformatting. I believe they cannot all usefully be avoided. As such, I am unsure that this option carries its weight of making the implementation more complex. How often does this happen for you? Why can't you just format the additional incorrect line? https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
acoomans added a comment. Also ping @jolesiak since he initially filed the report https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
acoomans added a comment. @djasper I updated the description of the diff. This fixes the issue of `clang-format -lines=x:x` not returning the same results as `clang-format`, while keeping the current behavior as default. https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
djasper added a comment. Could you explain what problem this is fixing? https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49580: [clang-format] Adding style option for absolute formatting
acoomans updated this revision to Diff 156571. acoomans retitled this revision from "[WIP] Change clang-format to absolute indentation" to "[clang-format] Adding style option for absolute formatting". acoomans edited the summary of this revision. https://reviews.llvm.org/D49580 Files: docs/ClangFormatStyleOptions.rst include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/UnwrappedLineFormatter.cpp test/Format/line-ranges-indent-abs.c test/Format/line-ranges-indent-abs.m test/Format/line-ranges-indent-rel.c Index: test/Format/line-ranges-indent-rel.c === --- /dev/null +++ test/Format/line-ranges-indent-rel.c @@ -0,0 +1,10 @@ +// RUN: grep -Ev "// *[A-Z-]+:" %s \ +// RUN: | clang-format -style=LLVM -lines=2:2 \ +// RUN: | FileCheck -strict-whitespace %s +// CHECK: {{^\ int f\(void\) \{$}} + int f(void) { +// CHECK: {{^\ \ \ int i = 0;$}} + int i = 0; +// CHECK: {{^\ \ \ return i;$}} + return i; +} Index: test/Format/line-ranges-indent-abs.m === --- /dev/null +++ test/Format/line-ranges-indent-abs.m @@ -0,0 +1,9 @@ +// RUN: grep -Ev "// *[A-Z-]+:" %s \ +// RUN: | clang-format -style="{BasedOnStyle: LLVM, IndentReference: Absolute}" -lines=2:2 \ +// RUN: | FileCheck -strict-whitespace %s +// CHECK: {{^\ \@protocol\ A$}} + @protocol A +// CHECK: {{^-\ \(void\)f;$}} +- (void)f; +// CHECK: {{^\@end$}} +@end Index: test/Format/line-ranges-indent-abs.c === --- /dev/null +++ test/Format/line-ranges-indent-abs.c @@ -0,0 +1,10 @@ +// RUN: grep -Ev "// *[A-Z-]+:" %s \ +// RUN: | clang-format -style="{BasedOnStyle: LLVM, IndentReference: Absolute}" -lines=2:2 \ +// RUN: | FileCheck -strict-whitespace %s +// CHECK: {{^\ int f\(void\) \{$}} + int f(void) { +// CHECK: {{^\ \ int i = 0;$}} + int i = 0; +// CHECK: {{^\ \ return i;$}} + return i; +} Index: lib/Format/UnwrappedLineFormatter.cpp === --- lib/Format/UnwrappedLineFormatter.cpp +++ lib/Format/UnwrappedLineFormatter.cpp @@ -1089,10 +1089,13 @@ format(Tok->Children, DryRun); // Adapt following lines on the current indent level to the same level - // unless the current \c AnnotatedLine is not at the beginning of a line. + // if indentation should be relative and unless the + // current \c AnnotatedLine is not at the beginning of a line. + bool RelativeIndentation = + Style.IndentReference != FormatStyle::IRS_Absolute; bool StartsNewLine = TheLine.First->NewlinesBefore > 0 || TheLine.First->IsFirst; - if (StartsNewLine) + if (RelativeIndentation && StartsNewLine) IndentTracker.adjustToUnmodifiedLine(TheLine); if (!DryRun) { bool ReformatLeadingWhitespace = Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -241,6 +241,13 @@ } }; +template <> struct ScalarEnumerationTraits { + static void enumeration(IO &IO, FormatStyle::IndentReferenceStyle &Value) { +IO.enumCase(Value, "Relative", FormatStyle::IRS_Relative); +IO.enumCase(Value, "Absolute", FormatStyle::IRS_Absolute); + } +}; + template <> struct ScalarEnumerationTraits { static void enumeration(IO &IO, FormatStyle::PointerAlignmentStyle &Value) { IO.enumCase(Value, "Middle", FormatStyle::PAS_Middle); @@ -411,6 +418,7 @@ IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex); IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels); IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives); +IO.mapOptional("IndentReference", Style.IndentReference); IO.mapOptional("IndentWidth", Style.IndentWidth); IO.mapOptional("IndentWrappedFunctionNames", Style.IndentWrappedFunctionNames); @@ -670,6 +678,7 @@ LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve; LLVMStyle.IndentCaseLabels = false; LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None; + LLVMStyle.IndentReference = FormatStyle::IRS_Relative; LLVMStyle.IndentWrappedFunctionNames = false; LLVMStyle.IndentWidth = 2; LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave; Index: include/clang/Format/Format.h === --- include/clang/Format/Format.h +++ include/clang/Format/Format.h @@ -1094,6 +1094,30 @@ /// The preprocessor directive indenting style to use. PPDirectiveIndentStyle IndentPPDirectives; + /// Different styles for indenting, used for partial formatting. + enum IndentReferenceStyle { +/// Indent relative to previous line, e.g.: +/// \code +/// int a = 0; +///int f(void) { // mis-indented by one space +/// int i = 0; <- those l