[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
MyDeveloperDay added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:2706 + + For example: STRINGIZE + JakeMerdichAMD wrote: > curdeius wrote: > > Shouldn't there be a configuration example like what's in `ForEachMacros` > > doc? > > ``` > > In the .clang-format configuration file, this can be configured like: > > > > .. code-block:: yaml > > > > WhitespaceSensitiveMacros: ['STRINGIZE', 'PP_STRINGIZE'] > > > > For example: BOOST_PP_STRINGIZE. > > ``` > > > Done. I also added PP_STRINGIZE and BOOST_PP_STRINGIZE as defaults; seems > reasonable. The change I think you made here gets lost when the ClangFormatOptions.rst file is regenerated Could you update the `Format.h` and regenerate rather than making the code changes in the rst itself? I ran into this issue when I tried to regenerate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82620/new/ https://reviews.llvm.org/D82620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0c332a7784c6: [clang-format] Preserve whitespace in selected
macros (authored by JakeMerdichAMD).
Changed prior to commit:
https://reviews.llvm.org/D82620?vs=273722&id=274102#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82620/new/
https://reviews.llvm.org/D82620
Files:
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/FormatToken.h
clang/lib/Format/FormatTokenLexer.cpp
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14021,6 +14021,19 @@
CHECK_PARSE("NamespaceMacros: [TESTSUITE, SUITE]", NamespaceMacros,
std::vector({"TESTSUITE", "SUITE"}));
+ Style.WhitespaceSensitiveMacros.clear();
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+ WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE, ASSERT]",
+ WhitespaceSensitiveMacros,
+ std::vector({"STRINGIZE", "ASSERT"}));
+ Style.WhitespaceSensitiveMacros.clear();
+ CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",
+ WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+ CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE', 'ASSERT']",
+ WhitespaceSensitiveMacros,
+ std::vector({"STRINGIZE", "ASSERT"}));
+
Style.IncludeStyle.IncludeCategories.clear();
std::vector ExpectedCategories = {
{"abc/.*", 2, 0}, {".*", 1, 0}};
@@ -16530,6 +16543,36 @@
verifyFormat("foo(operator, , -42);", Style);
}
+TEST_F(FormatTest, WhitespaceSensitiveMacros) {
+ FormatStyle Style = getLLVMStyle();
+ Style.WhitespaceSensitiveMacros.push_back("FOO");
+
+ // Don't use the helpers here, since 'mess up' will change the whitespace
+ // and these are all whitespace sensitive by definition
+ EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);", Style));
+ EXPECT_EQ(
+ "FOO(String-ized&Messy+But\\(: :Still)=Intentional);",
+ format("FOO(String-ized&Messy+But\\(: :Still)=Intentional);", Style));
+ EXPECT_EQ("FOO(String-ized&Messy+But,: :Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :Still=Intentional);", Style));
+ EXPECT_EQ("FOO(String-ized&Messy+But,: :\n"
+" Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :\n"
+ " Still=Intentional);",
+ Style));
+ Style.AlignConsecutiveAssignments = true;
+ EXPECT_EQ("FOO(String-ized=&Messy+But,: :\n"
+" Still=Intentional);",
+format("FOO(String-ized=&Messy+But,: :\n"
+ " Still=Intentional);",
+ Style));
+
+ Style.ColumnLimit = 21;
+ EXPECT_EQ("FOO(String-ized&Messy+But: :Still=Intentional);",
+format("FOO(String-ized&Messy+But: :Still=Intentional);", Style));
+}
+
TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
// These tests are not in NamespaceFixer because that doesn't
// test its interaction with line wrapping
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,27 @@
return false;
}
+ bool parseUntouchableParens() {
+while (CurrentToken) {
+ CurrentToken->Finalized = true;
+ switch (CurrentToken->Tok.getKind()) {
+ case tok::l_paren:
+next();
+if (!parseUntouchableParens())
+ return false;
+continue;
+ case tok::r_paren:
+next();
+return true;
+ default:
+// no-op
+break;
+ }
+ next();
+}
+return false;
+ }
+
bool parseParens(bool LookForDecls = false) {
if (!CurrentToken)
return false;
@@ -171,6 +192,11 @@
Contexts.back().ColonIsForRangeExpr =
Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
+if (Left->Previous && Left->Previous->is(TT_UntouchableMacroFunc)) {
+ Left->Finalized = true;
+ return parseUntouchableParens();
+}
+
bool StartsObjCMethodExpr = false;
if (FormatToken *MaybeSel = Left->Previous) {
// @selector( starts a selector.
@@ -1311,7 +1337,7 @@
TT_TypenameMacro, TT_FunctionLBrace, TT_ImplicitStringLiteral,
TT_InlineASMBrace, TT_JsFatArrow, TT_LambdaArrow, TT_NamespaceMacro,
TT_OverloadedOperator, TT_RegexLiteral, TT_TemplateString,
-TT_ObjCStringLiteral))
+TT_ObjCStringLiteral, TT_Untoucha
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
curdeius accepted this revision. curdeius added a comment. LGTM. Thank you for taking my comments into account. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82620/new/ https://reviews.llvm.org/D82620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82620/new/ https://reviews.llvm.org/D82620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
JakeMerdichAMD added a comment. Thanks for the fast review, @curdeius, and thanks for mentioning PP_STRINGIZE and BOOST_PP_STRINGIZE too! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82620/new/ https://reviews.llvm.org/D82620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
JakeMerdichAMD updated this revision to Diff 273722.
JakeMerdichAMD marked 3 inline comments as done.
JakeMerdichAMD added a comment.
Address feedback (nits, better docs, more defaults)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82620/new/
https://reviews.llvm.org/D82620
Files:
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/FormatToken.h
clang/lib/Format/FormatTokenLexer.cpp
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13957,6 +13957,18 @@
CHECK_PARSE("NamespaceMacros: [TESTSUITE, SUITE]", NamespaceMacros,
std::vector({"TESTSUITE", "SUITE"}));
+ Style.WhitespaceSensitiveMacros.clear();
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+ WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE, ASSERT]",
+ WhitespaceSensitiveMacros,
+ std::vector({"STRINGIZE", "ASSERT"}));
+ CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",
+ WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+ CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE', 'ASSERT']",
+ WhitespaceSensitiveMacros,
+ std::vector({"STRINGIZE", "ASSERT"}));
+
Style.IncludeStyle.IncludeCategories.clear();
std::vector ExpectedCategories = {
{"abc/.*", 2, 0}, {".*", 1, 0}};
@@ -16466,6 +16478,36 @@
verifyFormat("foo(operator, , -42);", Style);
}
+TEST_F(FormatTest, WhitespaceSensitiveMacros) {
+ FormatStyle Style = getLLVMStyle();
+ Style.WhitespaceSensitiveMacros.push_back("FOO");
+
+ // Don't use the helpers here, since 'mess up' will change the whitespace
+ // and these are all whitespace sensitive by definition
+ EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);", Style));
+ EXPECT_EQ(
+ "FOO(String-ized&Messy+But\\(: :Still)=Intentional);",
+ format("FOO(String-ized&Messy+But\\(: :Still)=Intentional);", Style));
+ EXPECT_EQ("FOO(String-ized&Messy+But,: :Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :Still=Intentional);", Style));
+ EXPECT_EQ("FOO(String-ized&Messy+But,: :\n"
+" Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :\n"
+ " Still=Intentional);",
+ Style));
+ Style.AlignConsecutiveAssignments = true;
+ EXPECT_EQ("FOO(String-ized=&Messy+But,: :\n"
+" Still=Intentional);",
+format("FOO(String-ized=&Messy+But,: :\n"
+ " Still=Intentional);",
+ Style));
+
+ Style.ColumnLimit = 21;
+ EXPECT_EQ("FOO(String-ized&Messy+But: :Still=Intentional);",
+format("FOO(String-ized&Messy+But: :Still=Intentional);", Style));
+}
+
TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
// These tests are not in NamespaceFixer because that doesn't
// test its interaction with line wrapping
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,27 @@
return false;
}
+ bool parseUntouchableParens() {
+while (CurrentToken) {
+ CurrentToken->Finalized = true;
+ switch (CurrentToken->Tok.getKind()) {
+ case tok::l_paren:
+next();
+if (!parseUntouchableParens())
+ return false;
+continue;
+ case tok::r_paren:
+next();
+return true;
+ default:
+// no-op
+break;
+ }
+ next();
+}
+return false;
+ }
+
bool parseParens(bool LookForDecls = false) {
if (!CurrentToken)
return false;
@@ -171,6 +192,11 @@
Contexts.back().ColonIsForRangeExpr =
Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
+if (Left->Previous && Left->Previous->is(TT_UntouchableMacroFunc)) {
+ Left->Finalized = true;
+ return parseUntouchableParens();
+}
+
bool StartsObjCMethodExpr = false;
if (FormatToken *MaybeSel = Left->Previous) {
// @selector( starts a selector.
@@ -1311,7 +1337,7 @@
TT_TypenameMacro, TT_FunctionLBrace, TT_ImplicitStringLiteral,
TT_InlineASMBrace, TT_JsFatArrow, TT_LambdaArrow, TT_NamespaceMacro,
TT_OverloadedOperator, TT_RegexLiteral, TT_TemplateString,
-TT_ObjCStringLiteral))
+TT_ObjCStringLiteral, TT_UntouchableMacroFunc))
CurrentToken->setType(TT_Unknown);
CurrentToken->Role.reset();
CurrentToken->MatchingParen = nullptr
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
JakeMerdichAMD marked 10 inline comments as done.
JakeMerdichAMD added inline comments.
Comment at: clang/docs/ClangFormatStyleOptions.rst:2706
+
+ For example: STRINGIZE
+
curdeius wrote:
> Shouldn't there be a configuration example like what's in `ForEachMacros` doc?
> ```
> In the .clang-format configuration file, this can be configured like:
>
> .. code-block:: yaml
>
> WhitespaceSensitiveMacros: ['STRINGIZE', 'PP_STRINGIZE']
>
> For example: BOOST_PP_STRINGIZE.
> ```
>
Done. I also added PP_STRINGIZE and BOOST_PP_STRINGIZE as defaults; seems
reasonable.
Comment at: clang/unittests/Format/FormatTest.cpp:13961
+ Style.WhitespaceSensitiveMacros.clear();
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+ WhitespaceSensitiveMacros,
std::vector{"STRINGIZE"});
curdeius wrote:
> Shouldn't that be:
> `CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",`
> as in other options that take vector of strings?
I'll add it since it ought to be tested, but they aren't required to my
knowledge due to 'YAML reasons' and most of the other tests omit them.
Comment at: clang/unittests/Format/FormatTest.cpp:16482
+ // and these are all whitespace sensitive by definition
+ EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);",
Style));
curdeius wrote:
> How about a test with escaped parentheses `\(` inside the macro argument?
Done. Note that parens always need to be matched anyhow, and the escaping
doesn't actually mean anything outside of a string literal (tested on MSVC,
gcc, clang), so no functionality change is needed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82620/new/
https://reviews.llvm.org/D82620
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
curdeius added inline comments.
Comment at: clang/docs/ClangFormatStyleOptions.rst:2706
+
+ For example: STRINGIZE
+
Shouldn't there be a configuration example like what's in `ForEachMacros` doc?
```
In the .clang-format configuration file, this can be configured like:
.. code-block:: yaml
WhitespaceSensitiveMacros: ['STRINGIZE', 'PP_STRINGIZE']
For example: BOOST_PP_STRINGIZE.
```
Comment at: clang/lib/Format/FormatTokenLexer.cpp:46-49
+ for (const std::string &WhitespaceSensitiveMacro :
+ Style.WhitespaceSensitiveMacros)
+Macros.insert(
+{&IdentTable.get(WhitespaceSensitiveMacro), TT_UntouchableMacroFunc});
Personally I would add braces around the loop body.
Comment at: clang/unittests/Format/FormatTest.cpp:13961
+ Style.WhitespaceSensitiveMacros.clear();
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+ WhitespaceSensitiveMacros,
std::vector{"STRINGIZE"});
Shouldn't that be:
`CHECK_PARSE("WhitespaceSensitiveMacros: ['STRINGIZE']",`
as in other options that take vector of strings?
Comment at: clang/unittests/Format/FormatTest.cpp:13963
+ WhitespaceSensitiveMacros,
std::vector{"STRINGIZE"});
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE, ASSERT]",
+ WhitespaceSensitiveMacros,
Ditto: apostrophes around strings.
Comment at: clang/unittests/Format/FormatTest.cpp:16482
+ // and these are all whitespace sensitive by definition
+ EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);",
Style));
How about a test with escaped parentheses `\(` inside the macro argument?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82620/new/
https://reviews.llvm.org/D82620
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82620: [clang-format] Preserve whitespace in selected macros
JakeMerdichAMD created this revision.
Herald added subscribers: cfe-commits, kristof.beyls.
Herald added a project: clang.
JakeMerdichAMD added reviewers: MyDeveloperDay, curdeius, sammccall, jbcoe.
JakeMerdichAMD added a project: clang-format.
https://bugs.llvm.org/show_bug.cgi?id=46383
When the c preprocessor stringizes tokens, the generated string literals
are affected by the whitespace. This means clang-format can affect
codegen silently, adding spaces and newlines to strings. Practically
speaking, the vast majority of cases will be harmless, only affecting
single identifiers or debug macros.
In the interest of doing no harm in other cases though, this introduces
a blacklist option 'WhitespaceSensitiveMacros', which contains a list of
names of function-like macros whose contents should not be touched by
clang-format, period. Clang-format can't automatically detect these
without a real compile context, so users will have to specify it
explicitly (it still beats clang-format off'ing at every invocation).
I added one default, "STRINGIZE", but am not particularly attached to
it, nor have I given much thought to defaults.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82620
Files:
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/FormatToken.h
clang/lib/Format/FormatTokenLexer.cpp
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13957,6 +13957,13 @@
CHECK_PARSE("NamespaceMacros: [TESTSUITE, SUITE]", NamespaceMacros,
std::vector({"TESTSUITE", "SUITE"}));
+ Style.WhitespaceSensitiveMacros.clear();
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE]",
+ WhitespaceSensitiveMacros, std::vector{"STRINGIZE"});
+ CHECK_PARSE("WhitespaceSensitiveMacros: [STRINGIZE, ASSERT]",
+ WhitespaceSensitiveMacros,
+ std::vector({"STRINGIZE", "ASSERT"}));
+
Style.IncludeStyle.IncludeCategories.clear();
std::vector ExpectedCategories = {
{"abc/.*", 2, 0}, {".*", 1, 0}};
@@ -16466,6 +16473,33 @@
verifyFormat("foo(operator, , -42);", Style);
}
+TEST_F(FormatTest, WhitespaceSensitiveMacros) {
+ FormatStyle Style = getLLVMStyle();
+ Style.WhitespaceSensitiveMacros.push_back("FOO");
+
+ // Don't use the helpers here, since 'mess up' will change the whitespace
+ // and these are all whitespace sensitive by definition
+ EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
+format("FOO(String-ized&Messy+But(: :Still)=Intentional);", Style));
+ EXPECT_EQ("FOO(String-ized&Messy+But,: :Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :Still=Intentional);", Style));
+ EXPECT_EQ("FOO(String-ized&Messy+But,: :\n"
+" Still=Intentional);",
+format("FOO(String-ized&Messy+But,: :\n"
+ " Still=Intentional);",
+ Style));
+ Style.AlignConsecutiveAssignments = true;
+ EXPECT_EQ("FOO(String-ized=&Messy+But,: :\n"
+" Still=Intentional);",
+format("FOO(String-ized=&Messy+But,: :\n"
+ " Still=Intentional);",
+ Style));
+
+ Style.ColumnLimit = 21;
+ EXPECT_EQ("FOO(String-ized&Messy+But: :Still=Intentional);",
+format("FOO(String-ized&Messy+But: :Still=Intentional);", Style));
+}
+
TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
// These tests are not in NamespaceFixer because that doesn't
// test its interaction with line wrapping
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,27 @@
return false;
}
+ bool parseUntouchableParens() {
+while (CurrentToken) {
+ CurrentToken->Finalized = true;
+ switch (CurrentToken->Tok.getKind()) {
+ case tok::l_paren:
+next();
+if (!parseUntouchableParens())
+ return false;
+continue;
+ case tok::r_paren:
+next();
+return true;
+ default:
+// no-op
+break;
+ }
+ next();
+}
+return false;
+ }
+
bool parseParens(bool LookForDecls = false) {
if (!CurrentToken)
return false;
@@ -171,6 +192,11 @@
Contexts.back().ColonIsForRangeExpr =
Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
+if (Left->Previous && Left->Previous->is(TT_UntouchableMacroFunc)) {
+ Left->Finalized = true;
+ return parseUntouchableParens();
+}
+
bool StartsObjCMethodExpr = false;
if (FormatToken *MaybeSel = Left->Previous) {
// @selector( starts a selector.
@@ -1311,7 +1337,7 @@
