Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Differential D138263 Diff 476344 clang/unittests/Format/FormatTestComments.cpp

Changeset View

Standalone View

View Options

clang/unittests/Format/FormatTestComments.cpp

Show First 20 Lines • Show All 62 Lines • ▼ Show 20 Linesprotected:
} }
void verifyFormat(llvm::StringRef Code, void verifyFormat(llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle()) { const FormatStyle &Style = getLLVMStyle()) {
EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable"; EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
EXPECT_EQ(Code.str(), format(test::messUp(Code), Style)); EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
} }
void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle()) {
EXPECT_EQ(Expected.str(), format(Expected, Style))
<< "Expected code is not stable";
EXPECT_EQ(Expected.str(), format(test::messUp(Code), Style));
}
void verifyFormatWithoutMessUp(llvm::StringRef Expected, llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle()) {
EXPECT_EQ(Expected.str(), format(Expected, Style))
<< "Expected code is not stable";
EXPECT_EQ(Expected.str(), format(Code, Style));
}
owenpanUnsubmitted
Done
ReplyInline Actions

Was this already done in D138402?

owenpan: Was this already done in D138402?
HazardyKnusperkeksAuthorUnsubmitted
Done
ReplyInline Actions

The first yes, the second no. I did just not update this diff, my commit is updated.

HazardyKnusperkeks: The first yes, the second no. I did just not update this diff, my commit is updated.
void verifyGoogleFormat(llvm::StringRef Code) { void verifyGoogleFormat(llvm::StringRef Code) {
verifyFormat(Code, getGoogleStyle()); verifyFormat(Code, getGoogleStyle());
} }
/// \brief Verify that clang-format does not crash on the given input. /// \brief Verify that clang-format does not crash on the given input.
void verifyNoCrash(llvm::StringRef Code, void verifyNoCrash(llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle()) { const FormatStyle &Style = getLLVMStyle()) {
format(Code, Style, SC_DoNotCheck); format(Code, Style, SC_DoNotCheck);
▲ Show 20 Lines • Show All 2,992 Lines • ▼ Show 20 Lines EXPECT_EQ("int foo = 12345; // comment\n"
" // comment which is\n" " // comment which is\n"
" // wrapped arround.\n", " // wrapped arround.\n",
format("int foo = 12345; // comment\n" format("int foo = 12345; // comment\n"
"int bar = 1234; // This is a very long comment\n" "int bar = 1234; // This is a very long comment\n"
" // which is wrapped arround.\n", " // which is wrapped arround.\n",
Style)); Style));
} }
TEST_F(FormatTestComments, DontAlignNamespaceComments) {
FormatStyle Style = getLLVMStyle();
owenpanUnsubmitted
Done
ReplyInline Actions

Should we add tests for FixNamespaceComments: false?

owenpan: Should we add tests for `FixNamespaceComments: false`?
HazardyKnusperkeksAuthorUnsubmitted
Done
ReplyInline Actions

Will do.

HazardyKnusperkeks: Will do.
Style.NamespaceIndentation = FormatStyle::NI_All;
Style.NamespaceMacros.push_back("TESTSUITE");
Style.ShortNamespaceLines = 0;
llvm::StringRef Input = "namespace A {\n"
owenpanUnsubmitted
Done
ReplyInline Actions
Style.ShortNamespaceLines = 0;
- llvm::StringRef Input = "namespace A {\n"
+ StringRef Input = "namespace A {\n"
" TESTSUITE(B) {\n"
owenpan:
" TESTSUITE(B) {\n"
" namespace C {\n"
" namespace D {} // namespace D\n"
" std::string Foo = Bar; // Comment\n"
" } // namespace C\n"
" }\n"
"} // NaMeSpAcE A";
EXPECT_EQ(Style.AlignTrailingComments.Kind, FormatStyle::TCAS_Always);
verifyFormat("namespace A {\n"
" TESTSUITE(B) {\n"
" namespace C {\n"
" namespace D {} // namespace D\n"
" std::string Foo = Bar; // Comment\n"
" } // namespace C\n"
" } // TESTSUITE(B)\n"
"} // NaMeSpAcE A",
Input, Style);
Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Never;
verifyFormat("namespace A {\n"
" TESTSUITE(B) {\n"
" namespace C {\n"
" namespace D {} // namespace D\n"
" std::string Foo = Bar; // Comment\n"
" } // namespace C\n"
" } // TESTSUITE(B)\n"
"} // NaMeSpAcE A",
Input, Style);
Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Leave;
verifyFormatWithoutMessUp("namespace A {\n"
" TESTSUITE(B) {\n"
" namespace C {\n"
" namespace D {} // namespace D\n"
" std::string Foo = Bar; // Comment\n"
" } // namespace C\n"
" }// TESTSUITE(B)\n"
owenpanUnsubmitted
Done
ReplyInline Actions

Why would TCAS_Leave result in no space before the trailing comment?

owenpan: Why would `TCAS_Leave` result in no space before the trailing comment?
HazardyKnusperkeksAuthorUnsubmitted
Done
ReplyInline Actions

It just did, didn't investigate or decide.
Most likely clang-format just adds it there and the space just comes from the other formatting, which is disabled with Leave. I'd say this is fine, Leave just means the coder decides on the position of the comments, and if that comment is added he can just move it around and clang-format will not touch it any further.

HazardyKnusperkeks: It just did, didn't investigate or decide. Most likely clang-format just adds it there and the…
owenpanUnsubmitted
Done
ReplyInline Actions

IMO there should be a single space or tab between the closing namespace brace and the namespace comment (existing or inserted) as AlignTrailingComments should not affect namespace comments. Anyway, we can fix it in another patch if necessary.

owenpan: IMO there should be a single space or tab between the closing `namespace` brace and the…
owenpanUnsubmitted
Done
ReplyInline Actions

See D156065.

owenpan: See D156065.
"} // NaMeSpAcE A",
Input, Style);
}
TEST_F(FormatTestComments, AlignsBlockCommentDecorations) { TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
EXPECT_EQ("/*\n" EXPECT_EQ("/*\n"
" */", " */",
format("/*\n" format("/*\n"
"*/", "*/",
getLLVMStyle())); getLLVMStyle()));
EXPECT_EQ("/*\n" EXPECT_EQ("/*\n"
" */", " */",
Show All 37 Lines EXPECT_EQ("/**\n"
format("/**\n" format("/**\n"
" * line */", " * line */",
getLLVMStyle())); getLLVMStyle()));
EXPECT_EQ("/**\n" EXPECT_EQ("/**\n"
" * line */", " * line */",
format("/**\n" format("/**\n"
" * line */", " * line */",
getLLVMStyle())); getLLVMStyle()));
EXPECT_EQ("/**\n" EXPECT_EQ("/**\n"
owenpanUnsubmitted
Done
ReplyInline Actions
"} // namespace A\n"
- " // Comment", Input, Style);
+ "// Comment",
+ Input, Style);
Style.FixNamespaceComments = false;

And put the test case in a #if 0 block? I don't think it should be aligned with the namespace comment.

owenpan: And put the test case in a `#if 0` block? I don't think it should be aligned with the…
HazardyKnusperkeksAuthorUnsubmitted
Done
ReplyInline Actions

I thought about it and came to the conclusion, that we (in this change) only want to suppress the alignment of namespace comments, not aligning other comments to the namespace comment.
But all is fine for me.

HazardyKnusperkeks: I thought about it and came to the conclusion, that we (in this change) only want to suppress…
" * line */", " * line */",
format("/**\n" format("/**\n"
" * line */", " * line */",
getLLVMStyle())); getLLVMStyle()));
EXPECT_EQ("/**\n" EXPECT_EQ("/**\n"
" * line */", " * line */",
format("/**\n" format("/**\n"
" * line */", " * line */",
owenpanUnsubmitted
Done
ReplyInline Actions
Style.FixNamespaceComments = false;
- verifyFormat("namespace A {\n"
- " int Foo;\n"
- " int Bar;\n"
- "}\n"
- "// Comment",
- Input, Style);
+ verifyFormat(Input, Style);
}
TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
owenpan:
getLLVMStyle())); getLLVMStyle()));
// Align the end '*/' after a line. // Align the end '*/' after a line.
EXPECT_EQ("/*\n" EXPECT_EQ("/*\n"
" * line\n" " * line\n"
" */", " */",
format("/*\n" format("/*\n"
"* line\n" "* line\n"
▲ Show 20 Lines • Show All 1,178 Lines • Show Last 20 Lines