[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Still waiting for feedback on this change! We've been happily using this patch at my company for some time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Hey, still motivated to land this, but definitely don't want to break any existing workflows. In particular, > but this afaict (unless I'm missing something :) will also affect the > workflow where the provided range is 0-length range I'm curious how to do this - as I mentioned, I know how to trigger a format on a 1-length range, but I don't understand how to trigger a 0-length range. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Sorry, coming back to this - line-filters are _inclusive_, so how do you indicate a 0-length range from the command line? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Thanks for the explanation! I do understand your philosophy on this, and agree with the desired behavior case you brought up where you have put in new braces. After thinking about this more, the thing I really care about is that clang-format is idempotent with a line filter - i.e., running it twice should always have the same effect as running it once. So, either this fix, or your proposed fix of fixing all lines until the next correct indentation would meet that idempotence criteria. However, I think in this particular case I still prefer my fix - to me, line filter is meant to limit the effect of clang-format to just fix a particular change and the fallout from that. However, if the lines _after_ a change were wrong before, this feels very unrelated to the change that was made - why is now the time to fix these unrelated lines? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. klimek: I'm sorry, I don't fully understand your proposed fix. Could you explain it in more detail? Without being able to respond to your proposal in detail, I strongly believe if you pass in a line range to clang-format, it should not change lines outside that range OR we should modify the documentation to clearly explain what the line filter option does. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. @MyDeveloperDay Thanks for the approve! Yes, this patch has been working for us as we've been using it on an internal fork since I opened the patch. As I mentioned, without this patch clang-format is useless for us in CI. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. @MyDeveloperDay Thanks for the approve! I'm not sure what the lingering concerns are, as far as I know there have been no concerns since october of last year. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. @djasper Do you have any further concerns about this patch? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Yes, I don’t have commit access and would need someone else to land the patch. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Sorry for the ambiguity. I meant, "running clang-format on the changed lines locally was not enough". I'll edit the description to clarify. To go in more detail, let's imagine you changed line 3. You run clang-format locally on line 3, but because of the bug fixed in the patch, this affects lines 3 and 4. Then, when you commit and push to CI, CI will see diffs on lines 3 and 4. CI will then try to ensure that running clang-format on line range 3-4 will produce no diffs. However, due to the same bug will produce a diff on line 5, so your code will be rejected by the CI. The reason our CI works like this is we want to ensure that no _new_ formatting errors are introduced into our code. We're extensive users of git blame and we don't want to do a full format pass on our whole codebase. Clang-format documents its line filter option, so in my opinion, it should work correctly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Ping! Still looking for help on this - I definitely don't want to diminish the complexity of this code, and would really appreciate any help getting this in. I've already apologized for the gap from feedback in July 2018 to response in October - and I'm happy to again - unfortunately sometimes life gets in the way! Please let me know if there's anything I can do to get this patch landed. As far as I understand, all outstanding comments on the code have been addressed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Bump! Still waiting feedback on this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Still looking for help here - thanks for the consideration. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Would really appreciate any feedback on this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Herald added a subscriber: jdoerfert. Bump! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Bump! Thanks for the consideration! Again, this is preventing us from rolling out automated style checking on diffs at work. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Still watching this space - thanks so much for your time! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Bump! Thanks for the consideration CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks for the consideration. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Thanks for the feedback! Does this mean that this won't be accepted? In my opinion, without these extra options, `AllowAllParametersOfDeclarationOnNextLine` is a very strange option. I don't think I'm the only one who feels this way, based on the stack overflow questions linked in the description. However, I fully understand if these changes are not wanted. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Thanks for the feedback! This actually isn't a new formatting option, rather it's fixing a bug where clang-format would change lines outside of the line range asked for by the user. This was preventing us from using clang-format in an automated setting. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Ping! Thanks for your consideration. I'm still quite motivated to land this, please let me know if there's anything I can do, or if it's an unwanted patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc marked 2 inline comments as done. russellmcc added a comment. Thanks for the feedback! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc updated this revision to Diff 181563.
russellmcc added a comment.
Respond to feedback
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54881/new/
https://reviews.llvm.org/D54881
Files:
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineFormatter.h
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -36,12 +36,12 @@
SC_DoNotCheck
};
- std::string format(llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+ std::string formatRange(llvm::StringRef Code, tooling::Range Range,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
LLVM_DEBUG(llvm::errs() << "---\n");
LLVM_DEBUG(llvm::errs() << Code << "\n\n");
-std::vector Ranges(1, tooling::Range(0, Code.size()));
+std::vector Ranges(1, std::move(Range));
FormattingAttemptStatus Status;
tooling::Replacements Replaces =
reformat(Style, Code, Ranges, "", &Status);
@@ -57,6 +57,13 @@
return *Result;
}
+ std::string format(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+return formatRange(Code, tooling::Range(0, Code.size()), Style,
+ CheckComplete);
+ }
+
FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
Style.ColumnLimit = ColumnLimit;
return Style;
@@ -110,6 +117,14 @@
format(Code, Style, SC_DoNotCheck);
}
+ void verifyWithPrefixAndSuffix(llvm::StringRef Expected, llvm::StringRef Code,
+ llvm::StringRef prefix, llvm::StringRef suffix,
+ const FormatStyle &Style = getLLVMStyle()) {
+EXPECT_EQ(llvm::Twine(prefix + Expected + suffix).str(),
+ formatRange(llvm::Twine(prefix + Code + suffix).str(),
+ tooling::Range(prefix.size(), Code.size()), Style));
+ }
+
int ReplacementCount;
};
@@ -9156,6 +9171,18 @@
Tab);
}
+TEST_F(FormatTest, FormattingIndentationDoesNotAffectOtherLines) {
+ FormatStyle Tab = getLLVMStyleWithColumns(42);
+ Tab.TabWidth = 4;
+ Tab.UseTab = FormatStyle::UT_Always;
+ verifyWithPrefixAndSuffix("\tfoobar();", "foobar();",
+"int f() {\nint a;\n", "\nint b;\n}\n",
+Tab);
+ Tab.UseTab = FormatStyle::UT_Never;
+ verifyWithPrefixAndSuffix("foobar();", "\tfoobar();",
+"int f() {\n\tint a;\n", "\n\tint b;\n}\n", Tab);
+}
+
TEST_F(FormatTest, CalculatesOriginalColumn) {
EXPECT_EQ("\"qq\\\n"
"q\"; /* some\n"
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -49,7 +49,8 @@
/// into tabs and spaces for some format styles.
void replaceWhitespace(FormatToken &Tok, unsigned Newlines, unsigned Spaces,
unsigned StartOfTokenColumn,
- bool InPPDirective = false);
+ bool InPPDirective = false,
+ bool OnlyUntilLastNewline = false);
/// Adds information about an unchangeable token's whitespace.
///
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -46,13 +46,23 @@
void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
unsigned Spaces,
unsigned StartOfTokenColumn,
- bool InPPDirective) {
+ bool InPPDirective,
+ bool OnlyUntilLastNewline) {
if (Tok.Finalized)
return;
Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
- Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
- Spaces, StartOfTokenColumn, Newlines, "", "",
- InPPDirective && !Tok.IsFirst,
+ SourceRange OriginalWhitespaceRange = Tok.WhitespaceRange;
+
+ if (OnlyUntilLastNewline) {
+OriginalWhitespaceRange.setEnd(
+OriginalWhitespaceRange.getBegin().getLocWithOffset(
+Tok.LastNewlineOffset));
+Spaces = 0;
+StartOfTokenColumn = 0;
+ }
+ Changes.push_back(Change(Tok, /*CreateReplacement=*/true,
+
[PATCH] D54881: Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc added a comment. Bump! Thanks for your consideration. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks again for your time. As far as I can tell, it's ready for another round of review! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks again for your consideration CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40988/new/ https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54881: Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc updated this revision to Diff 175185.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54881/new/
https://reviews.llvm.org/D54881
Files:
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineFormatter.h
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -36,12 +36,12 @@
SC_DoNotCheck
};
- std::string format(llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+ std::string formatRange(llvm::StringRef Code, tooling::Range Range,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
LLVM_DEBUG(llvm::errs() << "---\n");
LLVM_DEBUG(llvm::errs() << Code << "\n\n");
-std::vector Ranges(1, tooling::Range(0, Code.size()));
+std::vector Ranges(1, std::move(Range));
FormattingAttemptStatus Status;
tooling::Replacements Replaces =
reformat(Style, Code, Ranges, "", &Status);
@@ -57,6 +57,13 @@
return *Result;
}
+ std::string format(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+return formatRange(Code, tooling::Range(0, Code.size()), Style,
+ CheckComplete);
+ }
+
FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
Style.ColumnLimit = ColumnLimit;
return Style;
@@ -110,6 +117,14 @@
format(Code, Style, SC_DoNotCheck);
}
+ void verifyWithPrefixAndSuffix(llvm::StringRef Expected, llvm::StringRef Code,
+ llvm::StringRef prefix, llvm::StringRef suffix,
+ const FormatStyle &Style = getLLVMStyle()) {
+EXPECT_EQ(llvm::Twine(prefix + Expected + suffix).str(),
+ formatRange(llvm::Twine(prefix + Code + suffix).str(),
+ tooling::Range(prefix.size(), Code.size()), Style));
+ }
+
int ReplacementCount;
};
@@ -9141,6 +9156,7 @@
"\t */\n"
"\t int i;\n"
"}"));
+
Tab.AlignConsecutiveAssignments = true;
Tab.AlignConsecutiveDeclarations = true;
Tab.TabWidth = 4;
@@ -9156,6 +9172,18 @@
Tab);
}
+TEST_F(FormatTest, FormattingIndentationDoesNotAffectOtherLines) {
+ FormatStyle Tab = getLLVMStyleWithColumns(42);
+ Tab.TabWidth = 4;
+ Tab.UseTab = FormatStyle::UT_Always;
+ verifyWithPrefixAndSuffix("\tfoobar();", "foobar();",
+"int f() {\nint a;\n", "\nint b;\n}\n",
+Tab);
+ Tab.UseTab = FormatStyle::UT_Never;
+ verifyWithPrefixAndSuffix("foobar();", "\tfoobar();",
+"int f() {\n\tint a;\n", "\n\tint b;\n}\n", Tab);
+}
+
TEST_F(FormatTest, CalculatesOriginalColumn) {
EXPECT_EQ("\"qq\\\n"
"q\"; /* some\n"
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -49,7 +49,8 @@
/// into tabs and spaces for some format styles.
void replaceWhitespace(FormatToken &Tok, unsigned Newlines, unsigned Spaces,
unsigned StartOfTokenColumn,
- bool InPPDirective = false);
+ bool InPPDirective = false,
+ bool OnlyUntilLastNewline = false);
/// Adds information about an unchangeable token's whitespace.
///
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -46,13 +46,23 @@
void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
unsigned Spaces,
unsigned StartOfTokenColumn,
- bool InPPDirective) {
+ bool InPPDirective,
+ bool OnlyUntilLastNewline) {
if (Tok.Finalized)
return;
Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
- Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
- Spaces, StartOfTokenColumn, Newlines, "", "",
- InPPDirective && !Tok.IsFirst,
+ auto OriginalWhitespaceRange = Tok.WhitespaceRange;
+
+ if (OnlyUntilLastNewline) {
+OriginalWhitespaceRange.setEnd(
+OriginalWhitespaceRange.getBegin().getLocWithOffset(
+
[PATCH] D54881: Prevent Clang-Format from editing leading whitespace on lines outside of the format range
russellmcc created this revision.
russellmcc added a reviewer: djasper.
Herald added a subscriber: cfe-commits.
When clang-format is set to always use tabs (with tab width 4), when asked to
format line 3 (using the `-lines=3:3` flag):
int f() {
int a;
foobar();
int b;
}
We would expect clang-format to not modify the leading whitespace on line 4.
However, it does - see the new test. This is because the whitespace manager is
called to replace the whitespace before the first token of line 4. This is
necessary to edit the number of new lines before line 4, and to edit the
trailing whitespace on line 3. I've added a flag to `replaceWhitespace` that
allows it to not edit the leading whitespace, and only edit the whitespace up
to the last newline.
We ran into this when trying to integrate clang-format into our CI to ensure no
new formatting diffs were introduced in a patch. With the behavior without
this patch, the number of affected lines grew each time clang-format was run,
so running clang-format locally was not enough to ensure that CI would pass.
Repository:
rC Clang
https://reviews.llvm.org/D54881
Files:
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineFormatter.h
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -36,12 +36,12 @@
SC_DoNotCheck
};
- std::string format(llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+ std::string formatRange(llvm::StringRef Code, tooling::Range Range,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
LLVM_DEBUG(llvm::errs() << "---\n");
LLVM_DEBUG(llvm::errs() << Code << "\n\n");
-std::vector Ranges(1, tooling::Range(0, Code.size()));
+std::vector Ranges(1, std::move(Range));
FormattingAttemptStatus Status;
tooling::Replacements Replaces =
reformat(Style, Code, Ranges, "", &Status);
@@ -57,6 +57,13 @@
return *Result;
}
+ std::string format(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+return formatRange(Code, tooling::Range(0, Code.size()), Style,
+ CheckComplete);
+ }
+
FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
Style.ColumnLimit = ColumnLimit;
return Style;
@@ -110,6 +117,14 @@
format(Code, Style, SC_DoNotCheck);
}
+ void verifyWithPrefixAndSuffix(llvm::StringRef Expected, llvm::StringRef Code,
+ llvm::StringRef prefix, llvm::StringRef suffix,
+ const FormatStyle &Style = getLLVMStyle()) {
+EXPECT_EQ(llvm::Twine(prefix + Expected + suffix).str(),
+ formatRange(llvm::Twine(prefix + Code + suffix).str(),
+ tooling::Range(prefix.size(), Code.size()), Style));
+ }
+
int ReplacementCount;
};
@@ -9141,6 +9156,7 @@
"\t */\n"
"\t int i;\n"
"}"));
+
Tab.AlignConsecutiveAssignments = true;
Tab.AlignConsecutiveDeclarations = true;
Tab.TabWidth = 4;
@@ -9156,6 +9172,18 @@
Tab);
}
+TEST_F(FormatTest, FormattingIndentationDoesNotAffectOtherLines) {
+ FormatStyle Tab = getLLVMStyleWithColumns(42);
+ Tab.TabWidth = 4;
+ Tab.UseTab = FormatStyle::UT_Always;
+ verifyWithPrefixAndSuffix("\tfoobar();", "foobar();",
+"int f() {\nint a;\n", "\nint b;\n}\n",
+Tab);
+ Tab.UseTab = FormatStyle::UT_Never;
+ verifyWithPrefixAndSuffix("foobar();", "\tfoobar();",
+"int f() {\n\tint a;\n", "\n\tint b;\n}\n", Tab);
+}
+
TEST_F(FormatTest, CalculatesOriginalColumn) {
EXPECT_EQ("\"qq\\\n"
"q\"; /* some\n"
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -49,7 +49,8 @@
/// into tabs and spaces for some format styles.
void replaceWhitespace(FormatToken &Tok, unsigned Newlines, unsigned Spaces,
unsigned StartOfTokenColumn,
- bool InPPDirective = false);
+ bool InPPDirective = false,
+ bool OnlyUntilLastNewline = false);
/// Adds information about an unchangeable token's whitespace.
///
Index: lib/Format/WhitespaceManager.cpp
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks again for your consideration. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. bump! Thanks for your consideration. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Still looking for feedback on the latest round of changes. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks for your time https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks for your time! https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment.
Sorry for dropping this for so long! Stuff got busy at work and I've been
happily using my fork with this change for some time. I would really like this
to get in, and promise to be responsive to feedback.
Comment at: lib/Format/ContinuationIndenter.cpp:760
(!Style.AllowAllParametersOfDeclarationOnNextLine &&
State.Line->MustBeDeclaration) ||
+(!Style.AllowAllArgumentsOnNextLine &&
djasper wrote:
> russellmcc wrote:
> > djasper wrote:
> > > This still looks suspicious to me. State.Line->MustBeDeclaration is
> > > either true or false meaning that
> > > AllowAllParametersOfDeclarationOnNextLine or AllowAllArgumentsOnNextLine
> > > can always affect the behavior here, leading to BreakBeforeParameter to
> > > be set to true, even if we are in the case for
> > > PreviousIsBreakingCtorInitializerColon being true.
> > >
> > > So, my guess would be that if you set one of AllowAllArgumentsOnNextLine
> > > and AllowAllParametersOfDeclarationOnNextLine to false, then
> > > AllowAllConstructorInitializersOnNextLine doesn't have an effect anymore.
> > >
> > > Please verify, and if this is true, please fix and add tests. I think
> > > this might be easier to understand if you pulled the one if statement
> > > apart.
> > Actually, I think this logic works. The case you are worried about
> > (interaction between arguments, parameters, and constructor initializers)
> > is already tested in the unit tests in the
> > `AllowAllConstructorInitializersOnNextLine` test. The specific concern you
> > have is solved by the separate if statement below.
> >
> > I agree that this logic is a bit complex, but I think it's necessary since
> > in most cases we don't want to change the existing value of
> > `State.Stack.back().BreakBeforeParameter` - we only want to change this
> > when we are sure we should or shouldn't bin-pack. I've tried hard not to
> > change any existing behavior unless it was clearly a bug. I think we could
> > simplify this logic if we wanted to be less conservative.
> >
> > I'm not sure what you mean by breaking up the if statement - did you mean
> > something like this? To me, this reads much more verbose and is a bit more
> > confusing; however I'm happy to edit the diff if it makes more sense to you:
> >
> > ```
> > // If we are breaking after '(', '{', '<', or this is the break after a
> > ':'
> > // to start a member initializater list in a constructor, this should
> > not
> > // be considered bin packing unless the relevant AllowAll option is
> > false or
> > // this is a dict/object literal.
> > bool PreviousIsBreakingCtorInitializerColon =
> > Previous.is(TT_CtorInitializerColon) &&
> > Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon;
> >
> > if (!(Previous.isOneOf(tok::l_paren, tok::l_brace, TT_BinaryOperator) ||
> > PreviousIsBreakingCtorInitializerColon))
> > State.Stack.back().BreakBeforeParameter = true;
> >
> > if (!Style.AllowAllParametersOfDeclarationOnNextLine &&
> > State.Line->MustBeDeclaration)
> > State.Stack.back().BreakBeforeParameter = true;
> >
> > if (!Style.AllowAllArgumentsOnNextLine &&
> > !State.Line->MustBeDeclaration)
> > State.Stack.back().BreakBeforeParameter = true;
> >
> > if (!Style.AllowAllConstructorInitializersOnNextLine &&
> > PreviousIsBreakingCtorInitializerColon)
> > State.Stack.back().BreakBeforeParameter = true;
> >
> > if (Previous.is(TT_DictLiteral)))
> > State.Stack.back().BreakBeforeParameter = true;
> >
> > // If we are breaking after a ':' to start a member initializer list,
> > // and we allow all arguments on the next line, we should not break
> > // before the next parameter.
> > if (PreviousIsBreakingCtorInitializerColon &&
> > Style.AllowAllConstructorInitializersOnNextLine)
> > State.Stack.back().BreakBeforeParameter = false;
> > ```
> I find it hard to say whether you actually have a test for this. I'll make a
> suggestion on how to make these tests more maintainable below.
>
> I understand now how this works, but the specific case I was worried about is:
>
> AllowAllConstructorInitializersOnNextLine = true
> AllowAllArgumentsOnNextLine = false
> AllowAllParametersOfDeclarationOnNextLine = false
>
> (likely you only need one of the latter, but I am not sure which one :) )
>
> This works, because you are overwriting the value again in the subsequent if
> statement (which I hadn't seen).
>
> However, I do personally find that logic hard to reason about (if you have a
> sequence of if statements where some of them might overwrite the same value).
>
> Fundamentally, you are doing:
>
> if (something)
> value = true;
>
> if (otherthing)
> value = false;
>
> I think we don't care about (don't want to change) a pre-existing "value =
> true"
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc updated this revision to Diff 169288.
russellmcc added a comment.
Added suggested for loops to the test
https://reviews.llvm.org/D40988
Files:
docs/ClangFormatStyleOptions.rst
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3808,6 +3808,191 @@
"() {}"));
}
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ Style.BinPackParameters = false;
+
+ for (int i = 0; i < 4; ++i) {
+// Test all combinations of parameters that should not have an effect.
+Style.AllowAllParametersOfDeclarationOnNextLine = i & 1;
+Style.AllowAllArgumentsOnNextLine = i & 2;
+
+Style.AllowAllConstructorInitializersOnNextLine = true;
+Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor()\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+Style.AllowAllConstructorInitializersOnNextLine = true;
+verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor()\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+Style.AllowAllConstructorInitializersOnNextLine = true;
+verifyFormat("Constructor() :\n"
+ "aa(a), b(b) {}",
+ Style);
+
+Style.AllowAllConstructorInitializersOnNextLine = false;
+verifyFormat("Constructor() :\n"
+ "aa(a),\n"
+ "b(b) {}",
+ Style);
+ }
+
+ // Test interactions between AllowAllParametersOfDeclarationOnNextLine and
+ // AllowAllConstructorInitializersOnNextLine in all
+ // BreakConstructorInitializers modes
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int , int b)\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b,\n"
+ "int )\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = false;
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b)\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int , int b)\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b,\n"
+ "int )\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextL
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks again for your time. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks again for your feedback. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:760
(!Style.AllowAllParametersOfDeclarationOnNextLine &&
State.Line->MustBeDeclaration) ||
+(!Style.AllowAllArgumentsOnNextLine &&
djasper wrote:
> This still looks suspicious to me. State.Line->MustBeDeclaration is either
> true or false meaning that AllowAllParametersOfDeclarationOnNextLine or
> AllowAllArgumentsOnNextLine can always affect the behavior here, leading to
> BreakBeforeParameter to be set to true, even if we are in the case for
> PreviousIsBreakingCtorInitializerColon being true.
>
> So, my guess would be that if you set one of AllowAllArgumentsOnNextLine and
> AllowAllParametersOfDeclarationOnNextLine to false, then
> AllowAllConstructorInitializersOnNextLine doesn't have an effect anymore.
>
> Please verify, and if this is true, please fix and add tests. I think this
> might be easier to understand if you pulled the one if statement apart.
Actually, I think this logic works. The case you are worried about
(interaction between arguments, parameters, and constructor initializers) is
already tested in the unit tests in the
`AllowAllConstructorInitializersOnNextLine` test. The specific concern you
have is solved by the separate if statement below.
I agree that this logic is a bit complex, but I think it's necessary since in
most cases we don't want to change the existing value of
`State.Stack.back().BreakBeforeParameter` - we only want to change this when we
are sure we should or shouldn't bin-pack. I've tried hard not to change any
existing behavior unless it was clearly a bug. I think we could simplify this
logic if we wanted to be less conservative.
I'm not sure what you mean by breaking up the if statement - did you mean
something like this? To me, this reads much more verbose and is a bit more
confusing; however I'm happy to edit the diff if it makes more sense to you:
```
// If we are breaking after '(', '{', '<', or this is the break after a ':'
// to start a member initializater list in a constructor, this should not
// be considered bin packing unless the relevant AllowAll option is false or
// this is a dict/object literal.
bool PreviousIsBreakingCtorInitializerColon =
Previous.is(TT_CtorInitializerColon) &&
Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon;
if (!(Previous.isOneOf(tok::l_paren, tok::l_brace, TT_BinaryOperator) ||
PreviousIsBreakingCtorInitializerColon))
State.Stack.back().BreakBeforeParameter = true;
if (!Style.AllowAllParametersOfDeclarationOnNextLine &&
State.Line->MustBeDeclaration)
State.Stack.back().BreakBeforeParameter = true;
if (!Style.AllowAllArgumentsOnNextLine && !State.Line->MustBeDeclaration)
State.Stack.back().BreakBeforeParameter = true;
if (!Style.AllowAllConstructorInitializersOnNextLine &&
PreviousIsBreakingCtorInitializerColon)
State.Stack.back().BreakBeforeParameter = true;
if (Previous.is(TT_DictLiteral)))
State.Stack.back().BreakBeforeParameter = true;
// If we are breaking after a ':' to start a member initializer list,
// and we allow all arguments on the next line, we should not break
// before the next parameter.
if (PreviousIsBreakingCtorInitializerColon &&
Style.AllowAllConstructorInitializersOnNextLine)
State.Stack.back().BreakBeforeParameter = false;
```
https://reviews.llvm.org/D40988
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! Thanks for your consideration https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc marked an inline comment as not done. russellmcc added a comment. Okay; I think I've responded to all feedback at this point. Thanks for your patience guiding me through my first contribution to this project. Let me know what else I can do to help get this merged! https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc updated this revision to Diff 144280.
russellmcc marked an inline comment as done.
russellmcc added a comment.
Further update doc comments, render rst file
https://reviews.llvm.org/D40988
Files:
docs/ClangFormatStyleOptions.rst
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,168 @@
"() {}"));
}
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ Style.BinPackParameters = false;
+
+ verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+ verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("Constructor()\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+ verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int , int b)\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b,\n"
+ "int )\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = false;
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b)\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("Constructor()\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int , int b)\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b,\n"
+ "int )\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = false;
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b)\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("Constructor() :\n"
+ "aa(a), b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("Constructor() :\n"
+ "aa(a),\n"
+ "b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int , int b) :\n"
+ "(a),\n"
+ "b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyForm
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc updated this revision to Diff 144269.
russellmcc added a comment.
Responded to review feedback.
Improved documentation comments for new options.
While adding tests for the other constructor line break styles, I noticed an
inconsistency with the way "after colon" behaved with
`ConstructorInitializerAllOnOneLineOrOnePerLine`. Unlike the other two modes,
"after colon" breaking would never allow all initializers on a single line.
I've changed the behavior so the new
`AllowAllConstructorInitializersOnNextLine` is honored even in break after
colon mode. This does cause a slight change in behavior for default settings,
as now in after colon mode it will be allowed to put all the initializers in
one line with default settings. I've updated the unit tests for break after
colon mode to address this.
https://reviews.llvm.org/D40988
Files:
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,154 @@
"() {}"));
}
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ Style.BinPackParameters = false;
+
+ verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+ verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("Constructor()\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+ verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int , int b)\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b,\n"
+ "int )\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = false;
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b)\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("Constructor()\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int , int b)\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b,\n"
+ "int )\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllParametersOfDeclarationOnNextLine = false;
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("SomeClassWithALongName::Constructor(\n"
+ "int ,\n"
+ "int b)\n"
+ ": (a),\n"
+ " b(b) {}",
+ Style);
+
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("Constructor() :\n"
+ "aa(a), b(b) {}",
+
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump again! Any feedback would be quite appreciated. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bump! https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Bumping again - thanks so much for your time! https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. Ping! I believe all feedback has been addressed - further consideration would be much appreciated. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment.
Thanks for the feedback! I'm very motivated to get some support for these
features since they are required for my style guide. Let me know if my recent
changes made sense to you.
Comment at: lib/Format/ContinuationIndenter.cpp:756
State.Line->MustBeDeclaration) ||
-Previous.is(TT_DictLiteral))
+Previous.is(TT_DictLiteral) || !Style.AllowAllArgumentsOnNextLine)
State.Stack.back().BreakBeforeParameter = true;
djasper wrote:
> Hm, this looks suspicious. Doesn't this mean that AllowAllArgumentsOnNextLine
> implies/overwrites AllowAllParametersOfDeclarationOnNextLine?
>
> Now, there are two ways out of this:
> - Fix it
> - Provide different options
>
> The question is whether someone would ever want AllowAllArgumentsOnNextLine
> to be false while AllowAllParametersOfDeclarationOnNextLine is true. That
> would seem weird to me. If not, it might be better to turn the existing
> option into an enum with three values (None, Declarations, All) or something.
Oops! Thanks for finding this. I think since other options are exposed
separately for parameters and arguments (e.g., bin packing), it's more
consistent to break these out separately.
Comment at: lib/Format/ContinuationIndenter.cpp:962
State.Stack.back().NestedBlockIndent = State.Stack.back().Indent;
-if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
+if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine) {
State.Stack.back().AvoidBinPacking = true;
djasper wrote:
> I am not 100%, but I think this is doing too much. In accordance with the
> other options, this should still allow:
>
> Constructor() : a(a), b(b) {}
>
> so long as everything fits on one line (and I think it might not). Either
> way, add a test.
I think the case you're talking about actually works; I've added a test.
Comment at: lib/Format/Format.cpp:748
} else {
+ChromiumStyle.AllowAllArgumentsOnNextLine = true;
+ChromiumStyle.AllowAllConstructorInitializersOnNextLine = true;
djasper wrote:
> I think there is no need to set these here and below. Everything derives from
> LLVM style.
Removed!
Comment at: unittests/Format/FormatTest.cpp:3440
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
djasper wrote:
> If we go forward with this, the name of this option gets really confusing and
> we need to think about renaming it. Or maybe we can also turn it into an enum
> with three values?
>
> Here also, the combination of
> ConstructorInitializerAllOnOneLineOrOnePerLine=false and
> AllowAllConstructorInitializersOnNextLine=true
> seems really weird. I wouldn't even know what the desired behavior is in that
> case.
I agree this combination is weird, however the situation already existed with
declaration parameters (i.e., `AllowAllParametersOfDeclarationOnNextLine` has
no effect when `BinPackParameters` was true). I think exposing these new
parameters in this way is the most consistent with the existing approach.
I've edited the doc comment for `AllowAllConstructorInitializersOnNextLine ` to
note that it has no effect when
`ConstructorInitializerAllOnOneLineOrOnePerLine` is false.
https://reviews.llvm.org/D40988
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc updated this revision to Diff 133280.
russellmcc added a comment.
Responded to review feedback:
- Fix bug where `AllowAllArgumentsOnNextLine` overrode
`AllowAllParametersOfDeclarationOnNextLine`
- Add tests demonstrating independence of `AllowAllArgumentsOnNextLine` and
`AllowAllParametersOfDeclarationOnNextLine`
- Add test showing `AllowAllConstructorInitializersOnNextLine` doesn't affect
single-line constructors
- Removed unnecessary re-setting of llvm styles
- Removed bonus line from test
https://reviews.llvm.org/D40988
Files:
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,56 @@
"() {}"));
}
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+ verifyFormat("Constructor() : a(a), b(b) {}", Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("Constructor()\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+ verifyFormat("Constructor() : a(a), b(b) {}", Style);
+}
+
+TEST_F(FormatTest, AllowAllArgumentsOnNextLine) {
+ FormatStyle Style = getLLVMStyle();
+ Style.ColumnLimit = 60;
+ Style.BinPackArguments = false;
+ Style.AllowAllArgumentsOnNextLine = true;
+ verifyFormat("void foo() {\n"
+ " FunctionCallWithReallyLongName(\n"
+ " aaa, );\n"
+ "}",
+ Style);
+ Style.AllowAllArgumentsOnNextLine = false;
+ verifyFormat("void foo() {\n"
+ " FunctionCallWithReallyLongName(\n"
+ " aaa,\n"
+ " );\n"
+ "}",
+ Style);
+
+ // This parameter should not affect declarations.
+ Style.BinPackParameters = false;
+ Style.AllowAllParametersOfDeclarationOnNextLine = true;
+ verifyFormat("void FunctionCallWithReallyLongName(\n"
+ "int aaa, int );",
+ Style);
+ Style.AllowAllParametersOfDeclarationOnNextLine = false;
+ verifyFormat("void FunctionCallWithReallyLongName(\n"
+ "int aaa,\n"
+ "int );",
+ Style);
+}
+
TEST_F(FormatTest, BreakConstructorInitializersAfterColon) {
FormatStyle Style = getLLVMStyle();
Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
@@ -10020,6 +10070,8 @@
CHECK_PARSE_BOOL(AlignTrailingComments);
CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
+ CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
+ CHECK_PARSE_BOOL(AllowAllConstructorInitializersOnNextLine);
CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine);
CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -287,6 +287,10 @@
IO.mapOptional("AlignEscapedNewlines", Style.AlignEscapedNewlines);
IO.mapOptional("AlignOperands", Style.AlignOperands);
IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
+IO.mapOptional("AllowAllArgumentsOnNextLine",
+ Style.AllowAllArgumentsOnNextLine);
+IO.mapOptional("AllowAllConstructorInitializersOnNextLine",
+ Style.AllowAllConstructorInitializersOnNextLine);
IO.mapOptional("AllowAllParametersOfDeclarationOnNextLine",
Style.AllowAllParametersOfDeclarationOnNextLine);
IO.mapOptional("AllowShortBlocksOnASingleLine",
@@ -303,6 +307,7 @@
Style.AlwaysBreakAfterDefinitionReturnType);
IO.mapOptional("AlwaysBreakAfterReturnType",
Style.AlwaysBreakAfterReturnType);
+
// If AlwaysBreakAfterDefinitionReturnType was specified but
// AlwaysBreakAfterReturnType was not, initialize the latter from the
// former for backwards compatibility.
@@ -575,6 +580,8 @@
LLVMStyle.AlignTrailingComments = true;
LLVMStyle.AlignConsecutiveAssignments = false;
LLVMStyle.AlignConsecutiveDeclarations = false;
+ LLVMStyle.AllowAllArgumentsOnNextLine = true;
+ LLVMStyle.AllowAllCo
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc added a comment. ping! Thanks for your consideration Repository: rC Clang https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line
russellmcc created this revision.
Herald added a subscriber: klimek.
Add two new options,
AllowAllArgumentsOnNextLine and
AllowAllConstructorInitializersOnNextLine. These mirror the existing
AllowAllParametersOfDeclarationOnNextLine and allow me to support an
internal style guide where I work. I think this would be generally
useful, some have asked for it on stackoverflow:
https://stackoverflow.com/questions/30057534/clang-format-binpackarguments-not-working-as-expected
https://stackoverflow.com/questions/38635106/clang-format-how-to-prevent-all-function-arguments-on-next-line
Repository:
rC Clang
https://reviews.llvm.org/D40988
Files:
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3433,6 +3433,43 @@
"() {}"));
}
+TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ColumnLimit = 60;
+ Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+ Style.AllowAllConstructorInitializersOnNextLine = true;
+ verifyFormat("Constructor()\n"
+ ": (a), b(b) {}",
+ Style);
+
+ Style.AllowAllConstructorInitializersOnNextLine = false;
+ verifyFormat("Constructor()\n"
+ ": (a)\n"
+ ", b(b) {}",
+ Style);
+}
+
+TEST_F(FormatTest, AllowAllArgumentsOnNextLine) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ColumnLimit = 60;
+ Style.BinPackArguments = false;
+ Style.AllowAllArgumentsOnNextLine = true;
+ verifyFormat("void foo() {\n"
+ " FunctionCallWithReallyLongName(\n"
+ " aaa, );\n"
+ "}",
+ Style);
+ Style.AllowAllArgumentsOnNextLine = false;
+ verifyFormat("void foo() {\n"
+ " FunctionCallWithReallyLongName(\n"
+ " aaa,\n"
+ " );\n"
+ "}",
+ Style);
+}
+
TEST_F(FormatTest, BreakConstructorInitializersAfterColon) {
FormatStyle Style = getLLVMStyle();
Style.BreakConstructorInitializers = FormatStyle::BCIS_AfterColon;
@@ -10020,6 +10057,8 @@
CHECK_PARSE_BOOL(AlignTrailingComments);
CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
+ CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
+ CHECK_PARSE_BOOL(AllowAllConstructorInitializersOnNextLine);
CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine);
CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -287,6 +287,10 @@
IO.mapOptional("AlignEscapedNewlines", Style.AlignEscapedNewlines);
IO.mapOptional("AlignOperands", Style.AlignOperands);
IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
+IO.mapOptional("AllowAllArgumentsOnNextLine",
+ Style.AllowAllArgumentsOnNextLine);
+IO.mapOptional("AllowAllConstructorInitializersOnNextLine",
+ Style.AllowAllConstructorInitializersOnNextLine);
IO.mapOptional("AllowAllParametersOfDeclarationOnNextLine",
Style.AllowAllParametersOfDeclarationOnNextLine);
IO.mapOptional("AllowShortBlocksOnASingleLine",
@@ -303,6 +307,7 @@
Style.AlwaysBreakAfterDefinitionReturnType);
IO.mapOptional("AlwaysBreakAfterReturnType",
Style.AlwaysBreakAfterReturnType);
+
// If AlwaysBreakAfterDefinitionReturnType was specified but
// AlwaysBreakAfterReturnType was not, initialize the latter from the
// former for backwards compatibility.
@@ -575,6 +580,8 @@
LLVMStyle.AlignTrailingComments = true;
LLVMStyle.AlignConsecutiveAssignments = false;
LLVMStyle.AlignConsecutiveDeclarations = false;
+ LLVMStyle.AllowAllArgumentsOnNextLine = true;
+ LLVMStyle.AllowAllConstructorInitializersOnNextLine = true;
LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
LLVMStyle.AllowShortBlocksOnASingleLine = false;
@@ -738,6 +745,8 @@
ChromiumStyle.AllowShortIfStatementsOnASingleLine = false;
ChromiumStyle.AllowShortLoopsOnASingleLine = false;
} else {
+ChromiumStyle.AllowAllArgumentsOnNextLine = true;
+ChromiumStyle.AllowAllConstructorInitialize
