[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
