[clang-format] Separate License text and include blocks #77918
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format Author: serbanu (seranu) ChangesExtend SeparateDefinitionStyle to support spacing license text, include blocks and to also support two empty lines between blocks. Fixes #42112 . Patch is 24.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/77918.diff 5 Files Affected:
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 5ffd63ee73fc36..794817a26879d5 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -3853,46 +3853,51 @@ struct FormatStyle {
/// Leave definition blocks as they are.
SDS_Leave,
/// Insert an empty line between definition blocks.
- SDS_Always,
+ SDS_One,
+ /// Insert two empty lines between definition blocks.
+ SDS_Two,
/// Remove any empty line between definition blocks.
SDS_Never
};
/// Specifies the use of empty lines to separate definition blocks, including
- /// classes, structs, enums, and functions.
+ /// license text, includes, classes, structs, enums, and functions.
/// \code
/// Never v.s. Always
- /// #include <cstring> #include <cstring>
- /// struct Foo {
- /// int a, b, c; struct Foo {
- /// }; int a, b, c;
- /// namespace Ns { };
- /// class Bar {
- /// public: namespace Ns {
- /// struct Foobar { class Bar {
- /// int a; public:
- /// int b; struct Foobar {
- /// }; int a;
- /// private: int b;
- /// int t; };
- /// int method1() {
- /// // ... private:
- /// } int t;
- /// enum List {
- /// ITEM1, int method1() {
- /// ITEM2 // ...
- /// }; }
- /// template<typename T>
- /// int method2(T x) { enum List {
- /// // ... ITEM1,
- /// } ITEM2
- /// int i, j, k; };
- /// int method3(int par) {
- /// // ... template<typename T>
- /// } int method2(T x) {
- /// }; // ...
- /// class C {}; }
- /// }
+ /// /* License text /* License text
+ /// End license text */ End license text */
+ /// #include <cstring>
+ /// struct Foo { #include <cstring>
+ /// int a, b, c;
+ /// }; struct Foo {
+ /// namespace Ns { int a, b, c;
+ /// class Bar { };
+ /// public:
+ /// struct Foobar { namespace Ns {
+ /// int a; class Bar {
+ /// int b; public:
+ /// }; struct Foobar {
+ /// private: int a;
+ /// int t; int b;
+ /// int method1() { };
+ /// // ...
+ /// } private:
+ /// enum List { int t;
+ /// ITEM1,
+ /// ITEM2 int method1() {
+ /// }; // ...
+ /// template<typename T> }
+ /// int method2(T x) {
+ /// // ... enum List {
+ /// } ITEM1,
+ /// int i, j, k; ITEM2
+ /// int method3(int par) { };
+ /// // ...
+ /// } template<typename T>
+ /// }; int method2(T x) {
+ /// class C {}; // ...
+ /// } }
+ ///
/// int i, j, k;
///
/// int method3(int par) {
diff --git a/clang/lib/Format/DefinitionBlockSeparator.cpp b/clang/lib/Format/DefinitionBlockSeparator.cpp
index 319236d3bd618c..37d554fb678662 100644
--- a/clang/lib/Format/DefinitionBlockSeparator.cpp
+++ b/clang/lib/Format/DefinitionBlockSeparator.cpp
@@ -17,6 +17,22 @@
#include "llvm/Support/Debug.h"
#define DEBUG_TYPE "definition-block-separator"
+namespace {
+unsigned getNewlineCount(
+ clang::format::FormatStyle::SeparateDefinitionStyle separateDefinitions) {
+ switch (separateDefinitions) {
+ case clang::format::FormatStyle::SDS_One:
+ return 2;
+ case clang::format::FormatStyle::SDS_Two:
+ return 3;
+ case clang::format::FormatStyle::SDS_Never:
+ return 1;
+ case clang::format::FormatStyle::SDS_Leave:
+ assert(false);
+ }
+ return 1;
+}
+} // namespace
namespace clang {
namespace format {
std::pair<tooling::Replacements, unsigned> DefinitionBlockSeparator::analyze(
@@ -65,8 +81,7 @@ void DefinitionBlockSeparator::separateBlocks(
}
return false;
};
- unsigned NewlineCount =
- (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always ? 1 : 0) + 1;
+ unsigned NewlineCount = getNewlineCount(Style.SeparateDefinitionBlocks);
WhitespaceManager Whitespaces(
Env.getSourceManager(), Style,
Style.LineEnding > FormatStyle::LE_CRLF
@@ -74,9 +89,10 @@ void DefinitionBlockSeparator::separateBlocks(
Env.getSourceManager().getBufferData(Env.getFileID()),
Style.LineEnding == FormatStyle::LE_DeriveCRLF)
: Style.LineEnding == FormatStyle::LE_CRLF);
+ std::optional<bool> inLicenseText{};
for (unsigned I = 0; I < Lines.size(); ++I) {
const auto &CurrentLine = Lines[I];
- if (CurrentLine->InPPDirective)
+ if (CurrentLine->InMacroBody)
continue;
FormatToken *TargetToken = nullptr;
AnnotatedLine *TargetLine;
@@ -93,7 +109,7 @@ void DefinitionBlockSeparator::separateBlocks(
if (TargetToken->is(tok::eof))
return;
if (IsAccessSpecifierToken(TargetToken) ||
- (OpeningLineIndex > 0 &&
+ (OpeningLineIndex > 0 && OpeningLineIndex < Lines.size() &&
IsAccessSpecifierToken(Lines[OpeningLineIndex - 1]->First))) {
return;
}
@@ -171,6 +187,31 @@ void DefinitionBlockSeparator::separateBlocks(
return false;
};
+ // Separate License text.
+ const bool isComment = Lines[I]->isComment();
+ if (!inLicenseText.has_value()) {
+ inLicenseText = isComment;
+ if (isComment) {
+ while (I < Lines.size() && Lines[I]->isComment())
+ ++I;
+ if (I < Lines.size()) {
+ inLicenseText = false;
+ TargetLine = Lines[I];
+ TargetToken = TargetLine->First;
+ InsertReplacement(NewlineCount);
+ continue;
+ }
+ }
+ }
+
+ // Separate includes block.
+ if (I > 0 && Lines[I - 1]->isInclude() && !Lines[I]->isInclude()) {
+ TargetLine = Lines[I];
+ TargetToken = TargetLine->First;
+ InsertReplacement(NewlineCount);
+ continue;
+ }
+
if (HasEnumOnLine() &&
!LikelyDefinition(CurrentLine, /*ExcludeEnum=*/true)) {
// We have no scope opening/closing information for enum.
@@ -214,8 +255,10 @@ void DefinitionBlockSeparator::separateBlocks(
TargetToken = TargetLine->First;
if (!FollowingOtherOpening()) {
// Avoid duplicated replacement.
- if (TargetToken->isNot(tok::l_brace))
+ if (TargetToken->isNot(tok::l_brace) && OpeningLineIndex > 0 &&
+ !Lines[OpeningLineIndex - 1]->isInclude()) {
InsertReplacement(NewlineCount);
+ }
} else if (IsNeverStyle) {
InsertReplacement(OpeningLineIndex != 0);
}
@@ -238,7 +281,8 @@ void DefinitionBlockSeparator::separateBlocks(
++OpeningLineIndex;
}
TargetLine = Lines[OpeningLineIndex];
- if (!LikelyDefinition(TargetLine)) {
+ if (!LikelyDefinition(TargetLine) && OpeningLineIndex > 0 &&
+ !Lines[OpeningLineIndex - 1]->isInclude()) {
OpeningLineIndex = I + 1;
TargetLine = Lines[I + 1];
TargetToken = TargetLine->First;
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index ff5ed6c306f383..4e23fd8ed15e8f 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -586,7 +586,8 @@ template <>
struct ScalarEnumerationTraits<FormatStyle::SeparateDefinitionStyle> {
static void enumeration(IO &IO, FormatStyle::SeparateDefinitionStyle &Value) {
IO.enumCase(Value, "Leave", FormatStyle::SDS_Leave);
- IO.enumCase(Value, "Always", FormatStyle::SDS_Always);
+ IO.enumCase(Value, "One", FormatStyle::SDS_One);
+ IO.enumCase(Value, "Two", FormatStyle::SDS_Two);
IO.enumCase(Value, "Never", FormatStyle::SDS_Never);
}
};
diff --git a/clang/lib/Format/TokenAnnotator.h b/clang/lib/Format/TokenAnnotator.h
index 05a6daa87d8034..cf40eea9026270 100644
--- a/clang/lib/Format/TokenAnnotator.h
+++ b/clang/lib/Format/TokenAnnotator.h
@@ -113,6 +113,12 @@ class AnnotatedLine {
return First && First->is(tok::comment) && !First->getNextNonComment();
}
+ bool isInclude() const {
+ const auto *nextToken = First->getNextNonComment();
+ return First && First->is(tok::hash) && nextToken &&
+ nextToken->is(tok::pp_include);
+ }
+
/// \c true if this line starts with the given tokens in order, ignoring
/// comments.
template <typename... Ts> bool startsWith(Ts... Tokens) const {
diff --git a/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp b/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
index f5489498a93b9e..9f64675d796ada 100644
--- a/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
+++ b/clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
@@ -17,80 +17,97 @@
namespace clang {
namespace format {
namespace {
+std::string
+separateDefinitionBlocks(llvm::StringRef Code,
+ const std::vector<tooling::Range> &Ranges,
+ const FormatStyle &Style = getLLVMStyle()) {
+ LLVM_DEBUG(llvm::errs() << "---\n");
+ LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+ tooling::Replacements Replaces = reformat(Style, Code, Ranges, "<stdin>");
+ auto Result = applyAllReplacements(Code, Replaces);
+ EXPECT_TRUE(static_cast<bool>(Result));
+ LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+ return *Result;
+}
-class DefinitionBlockSeparatorTest : public ::testing::Test {
-protected:
- static std::string
- separateDefinitionBlocks(llvm::StringRef Code,
- const std::vector<tooling::Range> &Ranges,
- const FormatStyle &Style = getLLVMStyle()) {
- LLVM_DEBUG(llvm::errs() << "---\n");
- LLVM_DEBUG(llvm::errs() << Code << "\n\n");
- tooling::Replacements Replaces = reformat(Style, Code, Ranges, "<stdin>");
- auto Result = applyAllReplacements(Code, Replaces);
- EXPECT_TRUE(static_cast<bool>(Result));
- LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
- return *Result;
- }
-
- static std::string
- separateDefinitionBlocks(llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle()) {
- return separateDefinitionBlocks(
- Code,
- /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
- }
-
- static void _verifyFormat(const char *File, int Line, llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle(),
- llvm::StringRef ExpectedCode = "",
- bool Inverse = true) {
- ::testing::ScopedTrace t(File, Line, ::testing::Message() << Code.str());
- bool HasOriginalCode = true;
- if (ExpectedCode == "") {
- ExpectedCode = Code;
- HasOriginalCode = false;
- }
+std::string
+separateDefinitionBlocks(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle()) {
+ return separateDefinitionBlocks(
+ Code,
+ /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
+}
- EXPECT_EQ(ExpectedCode, separateDefinitionBlocks(ExpectedCode, Style))
- << "Expected code is not stable";
- if (Inverse) {
- FormatStyle InverseStyle = Style;
- if (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always)
- InverseStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Never;
- else
- InverseStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
- EXPECT_NE(ExpectedCode,
- separateDefinitionBlocks(ExpectedCode, InverseStyle))
- << "Inverse formatting makes no difference";
+std::string removeEmptyLines(llvm::StringRef Code) {
+ std::string Result = "";
+ for (auto Char : Code.str()) {
+ if (Result.size()) {
+ auto LastChar = Result.back();
+ if ((Char == '\n' && LastChar == '\n') ||
+ (Char == '\r' && (LastChar == '\r' || LastChar == '\n'))) {
+ continue;
+ }
}
- std::string CodeToFormat =
- HasOriginalCode ? Code.str() : removeEmptyLines(Code);
- std::string Result = separateDefinitionBlocks(CodeToFormat, Style);
- EXPECT_EQ(ExpectedCode, Result) << "Test failed. Formatted:\n" << Result;
+ Result.push_back(Char);
+ }
+ return Result;
+}
+void _verifyFormat(const char *File, int Line, llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ llvm::StringRef ExpectedCode = "", bool Inverse = true) {
+ ::testing::ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+ bool HasOriginalCode = true;
+ if (ExpectedCode == "") {
+ ExpectedCode = Code;
+ HasOriginalCode = false;
}
- static std::string removeEmptyLines(llvm::StringRef Code) {
- std::string Result = "";
- for (auto Char : Code.str()) {
- if (Result.size()) {
- auto LastChar = Result.back();
- if ((Char == '\n' && LastChar == '\n') ||
- (Char == '\r' && (LastChar == '\r' || LastChar == '\n'))) {
- continue;
- }
- }
- Result.push_back(Char);
+ EXPECT_EQ(ExpectedCode, separateDefinitionBlocks(ExpectedCode, Style))
+ << "Expected code is not stable";
+
+ auto checkInverseStyle = [&](FormatStyle::SeparateDefinitionStyle newStyle) {
+ FormatStyle InverseStyle = Style;
+ InverseStyle.SeparateDefinitionBlocks = newStyle;
+ if (newStyle == FormatStyle::SDS_Two)
+ InverseStyle.MaxEmptyLinesToKeep = 2;
+ EXPECT_NE(ExpectedCode,
+ separateDefinitionBlocks(ExpectedCode, InverseStyle))
+ << "Changing formatting makes no difference";
+ };
+ if (Inverse) {
+ switch (Style.SeparateDefinitionBlocks) {
+ case FormatStyle::SDS_Never:
+ checkInverseStyle(FormatStyle::SDS_One);
+ checkInverseStyle(FormatStyle::SDS_Two);
+ break;
+ case FormatStyle::SDS_One:
+ checkInverseStyle(FormatStyle::SDS_Never);
+ checkInverseStyle(FormatStyle::SDS_Two);
+ break;
+ case FormatStyle::SDS_Two:
+ checkInverseStyle(FormatStyle::SDS_Never);
+ checkInverseStyle(FormatStyle::SDS_One);
+ break;
+ case FormatStyle::SDS_Leave:
+ break;
}
- return Result;
}
-};
+ std::string CodeToFormat =
+ HasOriginalCode ? Code.str() : removeEmptyLines(Code);
+ std::string Result = separateDefinitionBlocks(CodeToFormat, Style);
+ EXPECT_EQ(ExpectedCode, Result) << "Test failed. Formatted:\n" << Result;
+}
+class DefinitionBlockSeparatorTest : public ::testing::Test {};
+
+class LicenseTest : public ::testing::TestWithParam<std::string> {};
+class IncludesTest : public ::testing::TestWithParam<std::string> {};
+class NoNewLineAtEofTest : public ::testing::TestWithParam<std::string> {};
#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
TEST_F(DefinitionBlockSeparatorTest, Basic) {
FormatStyle Style = getLLVMStyle();
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
verifyFormat("int foo(int i, int j) {\n"
" int r = i + j;\n"
" return r;\n"
@@ -164,7 +181,7 @@ TEST_F(DefinitionBlockSeparatorTest, Basic) {
TEST_F(DefinitionBlockSeparatorTest, FormatConflict) {
FormatStyle Style = getLLVMStyle();
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
llvm::StringRef Code = "class Test {\n"
"public:\n"
" static void foo() {\n"
@@ -178,7 +195,7 @@ TEST_F(DefinitionBlockSeparatorTest, FormatConflict) {
TEST_F(DefinitionBlockSeparatorTest, CommentBlock) {
FormatStyle Style = getLLVMStyle();
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
std::string Prefix = "enum Foo { FOO, BAR };\n"
"\n"
"/*\n"
@@ -248,18 +265,18 @@ TEST_F(DefinitionBlockSeparatorTest, UntouchBlockStartStyle) {
};
FormatStyle AlwaysStyle = getLLVMStyle();
- AlwaysStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ AlwaysStyle.SeparateDefinitionBlocks = FormatStyle::SDS_One;
FormatStyle NeverStyle = getLLVMStyle();
NeverStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Never;
- auto TestKit = MakeUntouchTest("/* FOOBAR */\n"
+ auto TestKit = MakeUntouchTest("/* FOOBAR */\n\n"
"#ifdef FOO\n\n",
"\n#elifndef BAR\n\n", "\n#endif\n\n", false);
verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
- TestKit = MakeUntouchTest("/* FOOBAR */\n"
+ TestKit = MakeUntouchTest("/* FOOBAR */\n\n"
"#ifdef FOO\n",
"#elifndef BAR\n", "#endif\n", false);
verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
@@ -282,7 +299,7 @@ TEST_F(DefinitionBlockSeparatorTest, UntouchBlockStartStyle) {
TEST_F(DefinitionBlockSeparatorTest, Always) {
FormatStyle Style = getLLVMStyle();
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
verifyFormat("// clang-format off\n"
"template<class T>\n"
@@ -400,7 +417,7 @@ TEST_F(DefinitionBlockSeparatorTest, Never) {
TEST_F(DefinitionBlockSeparatorTest, OpeningBracketOwnsLine) {
FormatStyle Style = getLLVMStyle();
Style.BreakBeforeBraces = FormatStyle::BS_Allman;
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
verifyFormat("namespace NS\n"
"{\n"
"// Enum test1\n"
@@ -464,7 +481,7 @@ TEST_F(DefinitionBlockSeparatorTest, OpeningBracketOwnsLine) {
TEST_F(DefinitionBlockSeparatorTest, TryBlocks) {
FormatStyle Style = getLLVMStyle();
Style.BreakBeforeBraces = FormatStyle::BS_Allman;
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
verifyFormat("void FunctionWithInternalTry()\n"
"{\n"
" try\n"
@@ -540,7 +557,7 @@ TEST_F(DefinitionBlockSeparatorTest, Leave) {
TEST_F(DefinitionBlockSeparatorTest, CSharp) {
FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Style.AllowShortEnumsOnASingleLine = false;
verifyFormat("namespace {\r\n"
@@ -581,7 +598,7 @@ TEST_F(DefinitionBlockSeparatorTest, CSharp) {
TEST_F(DefinitionBlockSeparatorTest, JavaScript) {
FormatStyle Style = getLLVMStyle(FormatStyle::LK_JavaScript);
- Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+ Style.SeparateDefinitionBlocks = FormatStyle::SDS_One;
Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Style.AllowShortEnumsOnASingleLine = false;
verifyFormat("export const enum Foo {\n"
@@ -610,6 +627,113 @@ TEST_F(DefinitionBlockSeparatorTest, JavaScript) {
"}",
Style);
}
+
+TEST_P(LicenseTest, SeparateLicenseFromBlock) {
+ constexpr StringRef LicenseSingleLineCommentStyle = {"// start license\n"
+ ...
[truncated]
|
There was a problem hiding this comment.
I think you want too much in one change.
Maybe first only handle #42112 for what I will be really grateful. And then the stuff with the license, because I see discussion there. As far as I can tell you just declare all comments on top of a file as license.
| Env.getSourceManager().getBufferData(Env.getFileID()), | ||
| Style.LineEnding == FormatStyle::LE_DeriveCRLF) | ||
| : Style.LineEnding == FormatStyle::LE_CRLF); | ||
| std::optional<bool> inLicenseText{}; |
There was a problem hiding this comment.
| std::optional<bool> inLicenseText{}; | |
| std::optional<bool> InLicenseText{}; |
| }; | ||
|
|
||
| // Separate License text. | ||
| const bool isComment = Lines[I]->isComment(); |
There was a problem hiding this comment.
| const bool isComment = Lines[I]->isComment(); | |
| const bool IsComment = Lines[I]->isComment(); |
All variables begin in upper case.
| Env.getSourceManager().getBufferData(Env.getFileID()), | ||
| Style.LineEnding == FormatStyle::LE_DeriveCRLF) | ||
| : Style.LineEnding == FormatStyle::LE_CRLF); | ||
| std::optional<bool> inLicenseText{}; |
There was a problem hiding this comment.
You don't use this as optional, just as plain bool (has_value), so something isn't right.
| return *Result; | ||
| } | ||
|
|
||
| class DefinitionBlockSeparatorTest : public ::testing::Test { |
There was a problem hiding this comment.
Why this massive change?
There was a problem hiding this comment.
I extracted the utility functions around _verifyFormat in the anonymous namespace so that they can be used by other test fixtures.
There was a problem hiding this comment.
Okay, this one I'd really like to be pulled out. And I'd want to hear from @owenca or @mydeveloperday. I'm not so fit in gtest, I can't say if we want this, or not.
There was a problem hiding this comment.
I extracted this change in a separate PR at #78108.
| IO.enumCase(Value, "One", FormatStyle::SDS_One); | ||
| IO.enumCase(Value, "Two", FormatStyle::SDS_Two); | ||
| IO.enumCase(Value, "Never", FormatStyle::SDS_Never); | ||
| } |
There was a problem hiding this comment.
You still need to handle "Always" for backwards compatibility.
There was a problem hiding this comment.
Does it need to be a spearate enum value or can it be handled as SDS_One?
There was a problem hiding this comment.
It will be SDS_One, it's just for the .clang-format file out there, which contain "Always". We provide compatibility for the config files, the users of libformat are not so lucky.
You'll find plenty of examples in other enums.
| IO.enumCase(Value, "One", FormatStyle::SDS_One); | ||
| IO.enumCase(Value, "Two", FormatStyle::SDS_Two); | ||
| IO.enumCase(Value, "Never", FormatStyle::SDS_Never); | ||
| } |
There was a problem hiding this comment.
You also need tests for parsing the option. Was this missed when added? Or didn't you run the test suite? At least currently "Always" should fail.
There was a problem hiding this comment.
I have been running check-clang-format clangFormat FormatTest and running FormatTest manually. Am I missing an important target to run to verify changes in clang-format?
Looking at ConfigParseTest.cpp I notice that there aren't any config parsing tests for SeparateDefinitionBlocks option. Will add.
There was a problem hiding this comment.
Thanks. That apparently was an oversight when the option was added.
#42112 requests separating both license text and include directives blocks.
In my proposal, the license text is considered indeed to be a block of comments at the top of the source file, definitely up for discussion.
How do you suggest I should split the work? |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
My bad, I only looked at the title, not the text.
You'd have to create multiple pull requests. And if they build on each other there is no nice way of doing this on github. You can of course use this one for a change. |
I created a new PR for the test refactoring at #78108 |
There was a problem hiding this comment.
Yeah I'm kind of a not aligned on this approach of hijacking what feels like the wrong setting, it feels complicated after the change which probably means we are not thinking about it enough about what people might request in the future
//
// License
//
<<-- added empty line
<<-- added empty line (NewLinesAfterLicense: 2)
#include <memory>
#include <string>
<<-- added empty line (NewLinesBetweenIncludeGroups: 2)
<<-- added empty line
#include <lib/memory>
#include <lib/string>
<<-- added empty line (NewLinesBetweenIncludeGroups: 2)
<<-- added empty line
#include <system/memory>
#include <system/string>
<<-- added empty line (NewLinesAfterIncludes: 3)
<<-- added empty line
<<-- added empty line
namespace Bar {
class Foo {
};} // Bar
clang/include/clang/Format/Format.h
Outdated
There was a problem hiding this comment.
- You cannot change Format.h without regenerating the Clang-Format-StyleOptions.rts
- I don't like SDS_One and SDS_Two they are very undescriptive for us in the code when we see it, it forces me to go look what it means
- I don't want to lose SDS_Always most of your code changes are a rename from One->Always
clang/include/clang/Format/Format.h
Outdated
There was a problem hiding this comment.
I am concerned by this approach, the others items there are often lost of items but for license block its likely one.
This feels like it should be handled seperately, with a new Options (coded seperately)
NewLinesAfterLicense: 2
NewLinesAfterIncludes: 1
I'd think about the Includes completely seperately as we might want to consider NewLines between IncludeGroups too.
But for me I think shoehorning this feature over the top of the SDS setting is unclear, and frankly a little smelly.
There was a problem hiding this comment.
Do you mean implementing separate TokenAnalizers for each option?
There was a problem hiding this comment.
Are we assuming any comment at the top of the file is a license comment?
There was a problem hiding this comment.
Yes, that was my working assumption
There was a problem hiding this comment.
Can we not write the test like this, I'm sorry but I think its so much more readable to say it all out in the verifyFormat
veryformat("// start license
....., Style);I don't want to have to run a debugger to understand the text thats being passed to verifyFormat I want to see it verbatum
There was a problem hiding this comment.
These tests are overly complex for what in my view you are trying to do... sorry I think its hard to read and doesn't matrch the style of the other tests in clang-format.
|
I created another PR to add the missing SeparateDefinitionBlocks config parse tests at #78256. I am planning to rework the change as per the review comments, but it's not yet clear to me what is the best way forward. I understand the need to have the options handled separately in code, but what I am not sure is:
|
seranu
force-pushed
the
separate-license-text-and-includes
branch
from
258aff2 to
8cc2aca
Compare
|
I updated the implementation and tried to include all the feedback I received. Regarding "NewLines between IncludeGroups" I think this is a separate option that, in my opinion, would be best implemented together with "IncludeBlocksStyle::IBS_Regroup" option. @mydeveloperday I'm not sure if this is what you expected, let me know what you think. |
seranu
force-pushed
the
separate-license-text-and-includes
branch
2 times, most recently
from
ae7d3b5 to
d98120c
Compare
…and EmptyLinesAfterIncludes
seranu
force-pushed
the
separate-license-text-and-includes
branch
from
d98120c to
af4bd7c
Compare
| /// | ||
| /// class Test {}; | ||
| /// \endcode | ||
| /// \version 1 |
There was a problem hiding this comment.
| /// \version 1 | |
| /// \version 18 |
You're a bit late for LLVM 1.0 ;)
There was a problem hiding this comment.
Somehow thought this was the version of the option.
| EmptyLinesAfterIncludes == R.EmptyLinesAfterIncludes && | ||
| EmptyLinesAfterTopLevelComment == R.EmptyLinesAfterTopLevelComment && | ||
| EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier && |
There was a problem hiding this comment.
| EmptyLinesAfterIncludes == R.EmptyLinesAfterIncludes && | |
| EmptyLinesAfterTopLevelComment == R.EmptyLinesAfterTopLevelComment && | |
| EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier && | |
| EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier && | |
| EmptyLinesAfterIncludes == R.EmptyLinesAfterIncludes && | |
| EmptyLinesAfterTopLevelComment == R.EmptyLinesAfterTopLevelComment && |
| /// class Test {}; | ||
| /// \endcode | ||
| /// \version 1 | ||
| std::optional<unsigned> EmptyLinesAfterTopLevelComment; |
There was a problem hiding this comment.
The name has to be improved.
// Top level comment in your definition
#include <foo>
// But this is also top level, or not?| /// level comment. Limited by MaxEmptyLinesToKeep. | ||
| /// Example: | ||
| /// EmptyLinesAfterTopLevelComment = 1 | ||
| /// \code |
There was a problem hiding this comment.
I prefer both examples in the same code block side by side. It is more concise.
| IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep); | ||
| IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation); | ||
| IO.mapOptional("NamespaceMacros", Style.NamespaceMacros); | ||
| IO.mapOptional("EmptyLinesAfterTopLevelComment", |
| if (CurrentLine->isComment()) { | ||
| InTopLevelComment = true; | ||
| } else if (InTopLevelComment) { | ||
| // Do not handle EOF newlines. | ||
| if (!CurrentLine->First->is(tok::eof) && CurrentLine->Affected) { | ||
| Whitespaces.replaceWhitespace(*CurrentLine->First, NewlineCount, | ||
| CurrentLine->First->OriginalColumn, | ||
| CurrentLine->First->OriginalColumn); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
#include <something>
// My comment
int foo();Is My comment now the "Top level" comment and new lines will be set accordingly? That is not what you intent to do, or am I mistaken?
There was a problem hiding this comment.
Another one
int foo() {
// Comment
return 5;
}| verifyFormat("#include <string>\n\n\n" | ||
| "class Test {};", |
There was a problem hiding this comment.
| verifyFormat("#include <string>\n\n\n" | |
| "class Test {};", | |
| verifyFormat("#include <string>\n" | |
| "\n" | |
| "\n" | |
| "class Test {};", |
So one does see the new lines directly.
| verifyFormat("#include <string>\n\n" | ||
| "class Test {};", | ||
| Style); | ||
| } |
There was a problem hiding this comment.
Please add tests with multiple includes and in conjunction with include sorting/grouping.
| "#include <iostream>\n" | ||
| "class Test {};", | ||
| Style); | ||
| } |
There was a problem hiding this comment.
Way more tests.
With declarations, statements, and what ever is possible before the first comment.
Also #ifndef include guards and `#pragma once'.
I'm going to close this MR and create two new ones.
Where can I find the changelog? |
|
|
I created a new PR for separating the includes at #78957 and will continue to work on getting the top-level comment code fixed as per review comments. Closing this PR. |
Extend SeparateDefinitionStyle to support spacing license text, include blocks and to also support two empty lines between blocks.
Fixes #42112 .