[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2b04c41b2832: [clang-format] Fix misannotation of colon in
presence of requires clause (authored by HazardyKnusperkeks).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129940/new/
https://reviews.llvm.org/D129940
Files:
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/TokenAnnotatorTest.cpp
Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -802,6 +802,70 @@
<< I;
}
}
+
+ BaseTokens = annotate("constexpr Foo(Foo const )\n"
+": value{other.value} {\n"
+" do_magic();\n"
+" do_more_magic();\n"
+"}");
+
+ ConstrainedTokens = annotate("constexpr Foo(Foo const )\n"
+ " requires std::is_copy_constructible\n"
+ ": value{other.value} {\n"
+ " do_magic();\n"
+ " do_more_magic();\n"
+ "}");
+
+ NumberOfBaseTokens = 26u;
+ NumberOfAdditionalRequiresClauseTokens = 7u;
+ NumberOfTokensBeforeRequires = 8u;
+
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
+ EXPECT_EQ(*BaseTokens[I], *ConstrainedTokens[I]) << I;
+} else {
+ EXPECT_EQ(*BaseTokens[I],
+*ConstrainedTokens[I + NumberOfAdditionalRequiresClauseTokens])
+ << I;
+}
+ }
+
+ BaseTokens = annotate("constexpr Foo(Foo const )\n"
+": value{other.value} {\n"
+" do_magic();\n"
+" do_more_magic();\n"
+"}");
+
+ ConstrainedTokens = annotate("constexpr Foo(Foo const )\n"
+ " requires (std::is_copy_constructible)\n"
+ ": value{other.value} {\n"
+ " do_magic();\n"
+ " do_more_magic();\n"
+ "}");
+
+ NumberOfBaseTokens = 26u;
+ NumberOfAdditionalRequiresClauseTokens = 9u;
+ NumberOfTokensBeforeRequires = 8u;
+
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
+ EXPECT_EQ(*BaseTokens[I], *ConstrainedTokens[I]) << I;
+} else {
+ EXPECT_EQ(*BaseTokens[I],
+*ConstrainedTokens[I + NumberOfAdditionalRequiresClauseTokens])
+ << I;
+}
+ }
}
TEST_F(TokenAnnotatorTest, UnderstandsAsm) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -999,7 +999,8 @@
FormatToken *Prev = Tok->getPreviousNonComment();
if (!Prev)
break;
-if (Prev->isOneOf(tok::r_paren, tok::kw_noexcept)) {
+if (Prev->isOneOf(tok::r_paren, tok::kw_noexcept) ||
+Prev->ClosesRequiresClause) {
Tok->setType(TT_CtorInitializerColon);
} else if (Prev->is(tok::kw_try)) {
// Member initializer list within function try block.
Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -802,6 +802,70 @@
<< I;
}
}
+
+ BaseTokens = annotate("constexpr Foo(Foo const )\n"
+": value{other.value} {\n"
+" do_magic();\n"
+" do_more_magic();\n"
+"}");
+
+ ConstrainedTokens = annotate("constexpr Foo(Foo const )\n"
+ " requires std::is_copy_constructible\n"
+ ": value{other.value} {\n"
+ " do_magic();\n"
+ " do_more_magic();\n"
+ "}");
+
+ NumberOfBaseTokens = 26u;
+ NumberOfAdditionalRequiresClauseTokens = 7u;
+ NumberOfTokensBeforeRequires = 8u;
+
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens +
[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
owenpan added inline comments.
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:820-833
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
curdeius wrote:
> owenpan wrote:
> > HazardyKnusperkeks wrote:
> > > curdeius wrote:
> > > > owenpan wrote:
> > > > > Can you make it a function or lambda?
> > > > :+1:
> > > Often thought about that. But as @MyDeveloperDay mentioned in different
> > > other reviews, we would loose the line where the EXPECT failed, since it
> > > would always be the same line.
> > >
> > > One step to mitigate that would be to return a `bool`, then one would
> > > loose the "subexpect", only knows which subtest failed.
> > >
> > > But an idea I have right now would be to add a StringRef parameter which
> > > is then fed into the expect/assert to identify the subtest.
> > Let's take care of this in a separate patch then. If a function or lambda
> > won't do, we can at least use a macro instead?
> You can then add a function taking a source location maybe (+ optionally a
> macro passing `__FILE__` and `__LINE__`). WDYT?
See D129982.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129940/new/
https://reviews.llvm.org/D129940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
owenpan accepted this revision.
owenpan added inline comments.
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:820-833
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
HazardyKnusperkeks wrote:
> curdeius wrote:
> > owenpan wrote:
> > > Can you make it a function or lambda?
> > :+1:
> Often thought about that. But as @MyDeveloperDay mentioned in different other
> reviews, we would loose the line where the EXPECT failed, since it would
> always be the same line.
>
> One step to mitigate that would be to return a `bool`, then one would loose
> the "subexpect", only knows which subtest failed.
>
> But an idea I have right now would be to add a StringRef parameter which is
> then fed into the expect/assert to identify the subtest.
Let's take care of this in a separate patch then. If a function or lambda won't
do, we can at least use a macro instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129940/new/
https://reviews.llvm.org/D129940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
curdeius added inline comments.
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:820-833
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
HazardyKnusperkeks wrote:
> curdeius wrote:
> > owenpan wrote:
> > > Can you make it a function or lambda?
> > :+1:
> Often thought about that. But as @MyDeveloperDay mentioned in different other
> reviews, we would loose the line where the EXPECT failed, since it would
> always be the same line.
>
> One step to mitigate that would be to return a `bool`, then one would loose
> the "subexpect", only knows which subtest failed.
>
> But an idea I have right now would be to add a StringRef parameter which is
> then fed into the expect/assert to identify the subtest.
You can then add a function taking a source location maybe (+ optionally a
macro passing `__FILE__` and `__LINE__`). WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129940/new/
https://reviews.llvm.org/D129940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
HazardyKnusperkeks added inline comments.
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:820-833
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
curdeius wrote:
> owenpan wrote:
> > Can you make it a function or lambda?
> :+1:
Often thought about that. But as @MyDeveloperDay mentioned in different other
reviews, we would loose the line where the EXPECT failed, since it would always
be the same line.
One step to mitigate that would be to return a `bool`, then one would loose the
"subexpect", only knows which subtest failed.
But an idea I have right now would be to add a StringRef parameter which is
then fed into the expect/assert to identify the subtest.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129940/new/
https://reviews.llvm.org/D129940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
curdeius accepted this revision.
curdeius added a comment.
Ok for me if it's OK for Owen.
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:820-833
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
owenpan wrote:
> Can you make it a function or lambda?
:+1:
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129940/new/
https://reviews.llvm.org/D129940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
owenpan added inline comments.
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:820-833
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
Can you make it a function or lambda?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129940/new/
https://reviews.llvm.org/D129940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129940: [clang-format] Fix misannotation of colon in presence of requires clause
HazardyKnusperkeks created this revision.
HazardyKnusperkeks added reviewers: MyDeveloperDay, curdeius, owenpan,
JohelEGP, eoanermine.
HazardyKnusperkeks added a project: clang-format.
Herald added a project: All.
HazardyKnusperkeks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
For clauses without parentheses it was annotated as TT_InheritanceColon.
Relates to https://github.com/llvm/llvm-project/issues/56215
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D129940
Files:
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/TokenAnnotatorTest.cpp
Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -799,6 +799,70 @@
<< I;
}
}
+
+ BaseTokens = annotate("constexpr Foo(Foo const )\n"
+": value{other.value} {\n"
+" do_magic();\n"
+" do_more_magic();\n"
+"}");
+
+ ConstrainedTokens = annotate("constexpr Foo(Foo const )\n"
+ " requires std::is_copy_constructible\n"
+ ": value{other.value} {\n"
+ " do_magic();\n"
+ " do_more_magic();\n"
+ "}");
+
+ NumberOfBaseTokens = 26u;
+ NumberOfAdditionalRequiresClauseTokens = 7u;
+ NumberOfTokensBeforeRequires = 8u;
+
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
+ EXPECT_EQ(*BaseTokens[I], *ConstrainedTokens[I]) << I;
+} else {
+ EXPECT_EQ(*BaseTokens[I],
+*ConstrainedTokens[I + NumberOfAdditionalRequiresClauseTokens])
+ << I;
+}
+ }
+
+ BaseTokens = annotate("constexpr Foo(Foo const )\n"
+": value{other.value} {\n"
+" do_magic();\n"
+" do_more_magic();\n"
+"}");
+
+ ConstrainedTokens = annotate("constexpr Foo(Foo const )\n"
+ " requires (std::is_copy_constructible)\n"
+ ": value{other.value} {\n"
+ " do_magic();\n"
+ " do_more_magic();\n"
+ "}");
+
+ NumberOfBaseTokens = 26u;
+ NumberOfAdditionalRequiresClauseTokens = 9u;
+ NumberOfTokensBeforeRequires = 8u;
+
+ ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+ ASSERT_EQ(ConstrainedTokens.size(),
+NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+ << ConstrainedTokens;
+
+ for (auto I = 0u; I < NumberOfBaseTokens; ++I) {
+if (I < NumberOfTokensBeforeRequires) {
+ EXPECT_EQ(*BaseTokens[I], *ConstrainedTokens[I]) << I;
+} else {
+ EXPECT_EQ(*BaseTokens[I],
+*ConstrainedTokens[I + NumberOfAdditionalRequiresClauseTokens])
+ << I;
+}
+ }
}
TEST_F(TokenAnnotatorTest, UnderstandsAsm) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -999,7 +999,8 @@
FormatToken *Prev = Tok->getPreviousNonComment();
if (!Prev)
break;
-if (Prev->isOneOf(tok::r_paren, tok::kw_noexcept)) {
+if (Prev->isOneOf(tok::r_paren, tok::kw_noexcept) ||
+Prev->ClosesRequiresClause) {
Tok->setType(TT_CtorInitializerColon);
} else if (Prev->is(tok::kw_try)) {
// Member initializer list within function try block.
Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -799,6 +799,70 @@
<< I;
}
}
+
+ BaseTokens = annotate("constexpr Foo(Foo const )\n"
+": value{other.value} {\n"
+" do_magic();\n"
+" do_more_magic();\n"
+"}");
+
+ ConstrainedTokens = annotate("constexpr Foo(Foo const )\n"
+ " requires std::is_copy_constructible\n"
+ ": value{other.value} {\n"
+ " do_magic();\n"
+ " do_more_magic();\n"
+ "}");
+
+ NumberOfBaseTokens = 26u;
+ NumberOfAdditionalRequiresClauseTokens = 7u;
+
