PR36643 Make clang-format support more generic TMarcos

[obfuscated]

Summary

This patch changes the check for _T to detect TMarcos with a more generic check.
This makes it possible to format wxWidgets projects (where wxT is used a lot) correctly.

Patch by Teodor Petrov

[alexfh]

We can't just treat anything("....") like the _T macro. There should be a whitelist configurable with an option. By default only _T should be handled.

[Typz]

A generic (or at least extandable) approach to specifying macro behaviors was introduced here: https://reviews.llvm.org/D33440

But this change seem to be on hold, as a configuration based on providing "macro definitions" seem to be preferable and kind-of in the pipe, though with no defined timeline...

[alexfh]

In D44765#1045394, @Typz wrote:

A generic (or at least extandable) approach to specifying macro behaviors was introduced here: https://reviews.llvm.org/D33440

I believe, that patch solves a significantly different problem and it won't make it much easier to implement correct handling of _T(x)-like macros (which expand to either x or L ## x depending on some other macro and thus have to be repeated for each fragment of the string literal after splitting).

The most extensible solution I see is to make the list of _T-like macro spellings configurable via an option.

[Typz]

You are right, I did not mean it would help with the handling of these macros; but it may be related on the configuration-side of things : adding an option for listing these macros may hit the same limitation, and the same mean of storing (in the code) the list of these macros may be used.

[obfuscated]

In D44765#1045373, @alexfh wrote:

We can't just treat anything("....") like the _T macro. There should be a whitelist configurable with an option. By default only _T should be handled.

What cases could break with this version of the patch?
Or you think it is just too dangerous to have this in such generic form?
I'm fine adding an option if this is deemed acceptable.

[alexfh]

In D44765#1045789, @obfuscated wrote:

In D44765#1045373, @alexfh wrote:

We can't just treat anything("....") like the _T macro. There should be a whitelist configurable with an option. By default only _T should be handled.

What cases could break with this version of the patch?

It is fine to split _T("veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery loooooooooooooooooooooooooooong striiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiing") as

_T("veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery ")
_T("loooooooooooooooooooooooooooong ")
_T("striiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiing")

and it's incorrect to split it like this:

_T("veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery "
   "loooooooooooooooooooooooooooong "
   "striiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiing")

since only the first string literal will get the L prefix in case when _T(x) is defined as L ## x. The same is valid for wxT() as I understand.

However, it's wrong to do so for any function or any macro which uses its argument in any way different from that of _T. E.g. strlen("aaa bbb") is not the same as

strlen("aaa ")
strlen("bbb")

I'm fine adding an option if this is deemed acceptable.

I'd say this should be an option.

[obfuscated]

Added an option

[obfuscated]

Attached both commits in a single diff...

[alexfh]

Thank you! One more comment from me and I'll leave the rest of the review to Krasimir, who has a better idea of how the configuration options should be named etc.

include/clang/Format/Format.h

1676–1677 ↗

(On Diff #139687)

I'm not sure "string wrapping macros" is the right term or can be easily understood by the reader of the code. The relevant MSDN page is talking about "generic-text mapping". I'm not sure this is a commonly used term either, but at least there's a precedent. Another relevant term is "encoding prefix". With these in mind, I could suggest following descriptions: "generic-text mapping macros", "string literal encoding prefix macros", "macros expanding to a string literal encoding prefix".

I'd also expand this a bit explaining where these macros are coming from and how they are special. Something along the lines of:

"Some libraries provide macro(s) to change the encoding prefix of string literals depending on configuration, for example, _T() macro on Microsoft platforms. When splitting string literals, the macro should be applied to each fragment of the literal to apply the same encoding prefix to all of them, which requires special treatment from clang-format. This option lists the names of these special macros."

[krasimir]

I don't have preferences over names, but I agree with Alex that the option should have more detailed description.

lib/Format/FormatToken.h

138

We don't strictly need this new field. We could do as in the old implementation and check if the string prefix is from a T macro in ContinuationIndenter.

unittests/Format/FormatTest.cpp

10692

Why is the FIXME here? I suggest just use the pattern similar to the other cases here and just keep the test with 2 elements:

Style.TMacros.clear();
std::vector<std::string> ExpectedTMacros = {"_T", "myT"};
CHECK_PARSE("TMacros: [_T, myT]", TMacros, ExpectedTMacros);

[obfuscated]

lib/Format/FormatToken.h

138

Using a flag is more reliable and faster - the checks are done once, so they don't have to be repeated.

From the field layout in the struct and from the usage of bools I could only conclude that this is not a memory nor performance critical part of the code.

unittests/Format/FormatTest.cpp

10692

I've copy pasted this from the foreach macro option.
I've not investigate why this fixme is there.

[obfuscated]

Fixed tests. Fixed description.

[krasimir]

lib/Format/FormatTokenLexer.cpp

385

In the original code, TMacro detection was done as:

(Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))

In the new code the left part is saved in TMacroStringLiteral, and the right part is checked in ContinuationIndenter. Could you keep them together in FormatTokenLexer.
@alexfh, why are we checking for the right check at all? What would be an example where this is needed to disambiguate?

[obfuscated]

lib/Format/FormatTokenLexer.cpp

385

Are you talking about the code in ContinuationIndenter::createBreakableToken?

I don't think I understand how the hole thing works.
Using the debugger I can see that this function is executed first and then createBreakableToken.
So we first detect the tmacro in FormatTokenLexer::tryMerge_TMacro and then use this information in the createBreakableToken to do something with it.

So when I get a TMacroStringLiteral==true in createBreakableToken I know that the token starts with a TMacro and there is no need to use some king of startswith calls. Probably the endswith call is redundant and I can do just a string search backwards...

[alexfh]

include/clang/Format/Format.h

1676 ↗

(On Diff #140191)

"A list of macro names"?

1686–1689 ↗

(On Diff #140191)

According to the description above, the option contains "a vector of macros", and here it says "these are expected to be macros of the form ...". This all is rather confusing. The above description should refer to names of macros, and this example should be made clearer in what it demonstrates.

lib/Format/FormatTokenLexer.cpp

371

Consider using llvm::find, which is a range adaptor for std::find.

385

In the new code the left part is saved in TMacroStringLiteral, and the right part is checked in ContinuationIndenter. Could you keep them together in FormatTokenLexer.

+1 to keeping these conditions together.

@alexfh, why are we checking for the right check at all? What would be an example where this is needed to disambiguate?

I don't know whether there's reasonable code that would be handled incorrectly without checking for the "), but I'm sure some test case can be generated, that would break clang-format badly without this condition. It also serves as a documentation (similar to asserts).

[obfuscated]

lib/Format/FormatTokenLexer.cpp

371

No idea what is range adaptor, but I can probably switch to it... :)

385

In the new code the left part is saved in TMacroStringLiteral, and the right part is checked in ContinuationIndenter. Could you keep them together in FormatTokenLexer.

+1 to keeping these conditions together.

Can you please explain what you mean here?
And what am I supposed to do, because I don't know how to put these conditions together.
Do you want me to search the tmacro vector in ContinuationIndenter::createBreakableToken instead of checking the bool flag?

[obfuscated]

Ping?

[alexfh]

lib/Format/FormatTokenLexer.cpp

371

It works with a range (something that has .begin() and .end()) instead of two iterators.

385

Looking at this code again, I see what the checks for _T(" and ") were needed for. If there was a space between _T( and " or between " and ), the code in createBreakableToken() would probably not work correctly (see the FIXME). tryMerge_TMacro() looks at tokens and doesn't care about the whitespace, so checking for TMacroStringLiteral is not equivalent to checking for <macro-name>(" and "). In particular, if you only remove just the first check, a test case like _T ( "...") may start being formatted in some way. We could remove the second check as well, if we manage to make formatting decent. Otherwise, performing these checks (that there are no spaces between _T, (, "..." and )) in tryMerge_TMacro() may be a better option than doing this in createBreakableToken().

lib/Format/ContinuationIndenter.cpp

if (Current.isStringLiteral()) {
 
     StringRef Text = Current.TokenText;
     StringRef Prefix;
     StringRef Postfix;
     // FIXME: Handle whitespace between '_T', '(', '"..."', and ')'.
     // FIXME: Store Prefix and Suffix (or PrefixLength and SuffixLength to
     // reduce the overhead) for each FormatToken, which is a string, so that we
     // don't run multiple checks here on the hot path.
-    if ((Text.endswith(Postfix = "\"") &&
-         (Text.startswith(Prefix = "@\"") || Text.startswith(Prefix = "\"") ||
-          Text.startswith(Prefix = "u\"") || Text.startswith(Prefix = "U\"") ||
-          Text.startswith(Prefix = "u8\"") ||
-          Text.startswith(Prefix = "L\""))) ||
-        (Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))) {
+    if (Text.endswith(Postfix = "\"") &&
+         Text.startswith(Prefix = "u8\"") || Text.startswith(Prefix = "L\""))) {
       // We need this to address the case where there is an unbreakable tail
       // only if certain other formatting decisions have been taken. The
       // UnbreakableTailLength of Current is an overapproximation is that case
       // and we need to be correct here.
       unsigned UnbreakableTailLength = (State.NextToken && canBreak(State))
                                            ? 0
                                            : Current.UnbreakableTailLength;
       return llvm::make_unique<BreakableStringLiteral>(
           Current, StartColumn, Prefix, Postfix, UnbreakableTailLength,
           State.Line->InPPDirective, Encoding, Style);
     }
+    if (Current.TMacroStringLiteral && Text.endswith(Postfix = "\")")) {
+      // If we're inside tmacro we know that we have something like _T(" at the
+      // beginning of the literal.
+      size_t pos = Text.find('"');
+      if (pos != StringRef::npos) {
+        Prefix = Text.substr(0, pos + 1);
+
+        // We need this to address the case where there is an unbreakable tail
+        // only if certain other formatting decisions have been taken. The
+        // UnbreakableTailLength of Current is an overapproximation is that case
+        // and we need to be correct here.
+        unsigned UnbreakableTailLength = (State.NextToken && canBreak(State))
+                                             ? 0
+                                             : Current.UnbreakableTailLength;
+        return llvm::make_unique<BreakableStringLiteral>(
+            Current, StartColumn, Prefix, Postfix, UnbreakableTailLength,
+            State.Line->InPPDirective, Encoding, Style);
+      }
+    }
   } else if (Current.is(TT_BlockComment)) {
     if (!Style.ReflowComments ||
         // If a comment token switches formatting, like
         // /* clang-format on */, we don't want to break it further,
         // but we may still want to adjust its indentation.
         switchesFormatting(Current)) {
       return nullptr;
     }

lib/Format/FormatToken.h

struct FormatToken {
   /// This can be used to determine what the user wrote in the original code
   /// and thereby e.g. leave an empty line between two function definitions.
   unsigned NewlinesBefore = 0;
 
   /// \brief Whether there is at least one unescaped newline before the \c
   /// Token.
   bool HasUnescapedNewline = false;
 
+  /// \brief Whether this is a string literal similar to _T("sdfsdf").
+  bool TMacroStringLiteral = false;
+
   /// \brief The range of the whitespace immediately preceding the \c Token.
   SourceRange WhitespaceRange;
 
   /// \brief The offset just past the last '\n' in this token's leading
   /// whitespace (relative to \c WhiteSpaceStart). 0 if there is no '\n'.
   unsigned LastNewlineOffset = 0;
 
   /// \brief The width of the non-whitespace parts of the token (or its first

lib/Format/FormatTokenLexer.cpp

bool FormatTokenLexer::tryMerge_TMacro() {
   FormatToken *String = Tokens[Tokens.size() - 2];
   if (!String->is(tok::string_literal) || String->IsMultiline)
     return false;
 
   if (!Tokens[Tokens.size() - 3]->is(tok::l_paren))
     return false;
 
   FormatToken *Macro = Tokens[Tokens.size() - 4];
-  if (Macro->TokenText != "_T")
+  if (Macro->TokenText.empty() || !String->TokenText.startswith("\"") || !String->TokenText.endswith("\""))
     return false;
 
   const char *Start = Macro->TokenText.data();
   const char *End = Last->TokenText.data() + Last->TokenText.size();
   String->TokenText = StringRef(Start, End - Start);
   String->IsFirst = Macro->IsFirst;
   String->LastNewlineOffset = Macro->LastNewlineOffset;
   String->WhitespaceRange = Macro->WhitespaceRange;
   String->OriginalColumn = Macro->OriginalColumn;
   String->ColumnWidth = encoding::columnWidthWithTabs(
       String->TokenText, String->OriginalColumn, Style.TabWidth, Encoding);
   String->NewlinesBefore = Macro->NewlinesBefore;
   String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+  String->TMacroStringLiteral = true;
 
   Tokens.pop_back();
   Tokens.pop_back();
   Tokens.pop_back();
   Tokens.back() = String;
   return true;
 }
 

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BreaksStringLiteralsWithin_TMacro) {
   EXPECT_EQ("f(\n"
             "\n"
             "    _T(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\"));",
             format("f(\n"
                    "\n"
                    "_T(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\"));"));
 }
 
+TEST_F(FormatTest, BreaksStringLiteralsWithin_GenericTMacro) {
+  FormatStyle Style = getLLVMStyleWithColumns(25);
+  EXPECT_EQ(
+      "blablaT(\"aaaaaaaaaaaaaa\")\n"
+      "blablaT(\"aaaaaaaaaaaaaa\")\n"
+      "blablaT(\"aaaaaaaaaaaa\")",
+      format("  blablaT(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\")", Style));
+  EXPECT_EQ("f(x,\n"
+            "  blablaT(\"aaaaaaaaaaaa\")\n"
+            "  blablaT(\"aaa\"),\n"
+            "  z);",
+            format("f(x, blablaT(\"aaaaaaaaaaaaaaa\"), z);", Style));
+
+  // FIXME: Handle embedded spaces in one iteration.
+  //  EXPECT_EQ("blablaT(\"aaaaaaaaaaaaa\")\n"
+  //            "blablaT(\"aaaaaaaaaaaaa\")\n"
+  //            "blablaT(\"aaaaaaaaaaaaa\")\n"
+  //            "blablaT(\"a\")",
+  //            format("  blablaT ( \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\" )",
+  //                   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ(
+      "blablaT ( \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\" )",
+      format("  blablaT ( \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\" )", Style));
+  EXPECT_EQ("f(\n"
+            "#if !TEST\n"
+            "    blablaT(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\")\n"
+            "#endif\n"
+            ");",
+            format("f(\n"
+                   "#if !TEST\n"
+                   "blablaT(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\")\n"
+                   "#endif\n"
+                   ");"));
+  EXPECT_EQ("f(\n"
+            "\n"
+            "    blablaT(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\"));",
+            format("f(\n"
+                   "\n"
+                   "blablaT(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\"));"));
+}
+
 TEST_F(FormatTest, BreaksStringLiteralOperands) {
   // In a function call with two operands, the second can be broken with no line
   // break before it.
   EXPECT_EQ("func(a, \"long long \"\n"
             "        \"long long\");",
             format("func(a, \"long long long long\");",
                    getLLVMStyleWithColumns(24)));
   // In a function call with three operands, the second must be broken with a
CHECK_PARSE("RawStringFormats:\n"
               "      - 'cc'\n"
               "      - 'cpp'\n"
               "    EnclosingFunctions:\n"
               "      - 'C_CODEBLOCK'\n"
               "      - 'CPPEVAL'\n"
               "    CanonicalDelimiter: 'cc'",
               RawStringFormats, ExpectedRawStringFormats);
 }
 
 TEST_F(FormatTest, ParsesConfigurationWithLanguages) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE("Language: Cpp\n"
               "IndentWidth: 12",
               IndentWidth, 12u);
   EXPECT_EQ(parseConfiguration("Language: JavaScript\n"
                                "IndentWidth: 34",