[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated created this revision.
obfuscated added a reviewer: krasimir.
Herald added a subscriber: klimek.

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


Repository:
  rC Clang

https://reviews.llvm.org/D44765

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7935,6 +7935,47 @@
"_T(\"Xn\"));"));
 }
 
+TEST_F(FormatTest, BreaksStringLiteralsWithin_GenericTMacro) {
+  FormatStyle Style = getLLVMStyleWithColumns(25);
+  EXPECT_EQ(
+  "blablaT(\"aa\")\n"
+  "blablaT(\"aa\")\n"
+  "blablaT(\"\")",
+  format("  blablaT(\"\")", Style));
+  EXPECT_EQ("f(x,\n"
+"  blablaT(\"\")\n"
+"  blablaT(\"aaa\"),\n"
+"  z);",
+format("f(x, blablaT(\"aaa\"), z);", Style));
+
+  // FIXME: Handle embedded spaces in one iteration.
+  //  EXPECT_EQ("blablaT(\"a\")\n"
+  //"blablaT(\"a\")\n"
+  //"blablaT(\"a\")\n"
+  //"blablaT(\"a\")",
+  //format("  blablaT ( \"\" )",
+  //   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ(
+  "blablaT ( \"\" )",
+  format("  blablaT ( \"\" )", Style));
+  EXPECT_EQ("f(\n"
+"#if !TEST\n"
+"blablaT(\"Xn\")\n"
+"#endif\n"
+");",
+format("f(\n"
+   "#if !TEST\n"
+   "blablaT(\"Xn\")\n"
+   "#endif\n"
+   ");"));
+  EXPECT_EQ("f(\n"
+"\n"
+"blablaT(\"Xn\"));",
+format("f(\n"
+   "\n"
+   "blablaT(\"Xn\"));"));
+}
+
 TEST_F(FormatTest, BreaksStringLiteralOperands) {
   // In a function call with two operands, the second can be broken with no line
   // break before it.
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -368,7 +368,7 @@
 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();
@@ -382,6 +382,7 @@
   String->TokenText, String->OriginalColumn, Style.TabWidth, Encoding);
   String->NewlinesBefore = Macro->NewlinesBefore;
   String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+  String->TMacroStringLiteral = true;
 
   Tokens.pop_back();
   Tokens.pop_back();
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -134,6 +134,9 @@
   /// 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;
 
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1599,12 +1599,10 @@
 // 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 = "@\"") || Text.startswith(Prefix = "\"") ||
+ Text.startswith(Prefix = "u\"") || Text.startswith(Prefix = "U\"") ||
+ Text.startswith(Prefix = "u8\"") || Text.startswith(Prefix = "L\""))) {
   // We need this to address the case where there is an unbreakable tail
   // only if certain 

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated added a comment.

In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D44765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated updated this revision to Diff 139686.
obfuscated added a comment.

Added an option


Repository:
  rC Clang

https://reviews.llvm.org/D44765

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatTokenLexer.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7937,6 +7937,7 @@
 
 TEST_F(FormatTest, BreaksStringLiteralsWithin_GenericTMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(25);
+  Style.TMacros.push_back("blablaT");
   EXPECT_EQ(
   "blablaT(\"aa\")\n"
   "blablaT(\"aa\")\n"
@@ -10688,6 +10689,17 @@
   "  - 'CPPEVAL'\n"
   "CanonicalDelimiter: 'cc'",
   RawStringFormats, ExpectedRawStringFormats);
+
+  // FIXME: This is required because parsing a configuration simply overwrites
+  // the first N elements of the list instead of resetting it.
+  Style.TMacros.clear();
+  std::vector TestTMarco;
+  TestTMarco.push_back("_T");
+  CHECK_PARSE("TMacros: [_T]", TMacros, TestTMarco);
+  std::vector TestTMarcoAndMyT;
+  TestTMarcoAndMyT.push_back("_T");
+  TestTMarcoAndMyT.push_back("myT");
+  CHECK_PARSE("TMacros: [_T, myT]", TMacros, TestTMarcoAndMyT);
 }
 
 TEST_F(FormatTest, ParsesConfigurationWithLanguages) {
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -368,7 +368,8 @@
 return false;
 
   FormatToken *Macro = Tokens[Tokens.size() - 4];
-  if (Macro->TokenText.empty() || !String->TokenText.startswith("\"") || 
!String->TokenText.endswith("\""))
+  if (std::find(Style.TMacros.begin(), Style.TMacros.end(), Macro->TokenText) 
==
+  Style.TMacros.end())
 return false;
 
   const char *Start = Macro->TokenText.data();
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -430,6 +430,7 @@
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("Standard", Style.Standard);
 IO.mapOptional("TabWidth", Style.TabWidth);
+IO.mapOptional("TMacros", Style.TMacros);
 IO.mapOptional("UseTab", Style.UseTab);
   }
 };
@@ -686,6 +687,7 @@
   LLVMStyle.DisableFormat = false;
   LLVMStyle.SortIncludes = true;
   LLVMStyle.SortUsingDeclarations = true;
+  LLVMStyle.TMacros.push_back("_T");
 
   return LLVMStyle;
 }
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -1673,6 +1673,22 @@
   /// \brief The number of columns used for tab stops.
   unsigned TabWidth;
 
+  /// \brief A vector of macros that should be interpreted as string wrapping
+  /// macros instead of as function calls.
+  ///
+  /// These are expected to be macros of the form:
+  /// \code
+  ///   _T("...some string...")
+  /// \endcode
+  ///
+  /// In the .clang-format configuration file, this can be configured like:
+  /// \code{.yaml}
+  ///   TMarcos: ['_T', 'myT']
+  /// \endcode
+  ///
+  /// For example: _T.
+  std::vector TMacros;
+
   /// \brief Different ways to use tab in formatting.
   enum UseTabStyle {
 /// Never use tab.
@@ -1781,7 +1797,7 @@
SpacesInParentheses == R.SpacesInParentheses &&
SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
Standard == R.Standard && TabWidth == R.TabWidth &&
-   UseTab == R.UseTab;
+   TMacros == R.TMacros && UseTab == R.UseTab;
   }
 
   llvm::Optional GetLanguageStyle(LanguageKind Language) const;


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7937,6 +7937,7 @@
 
 TEST_F(FormatTest, BreaksStringLiteralsWithin_GenericTMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(25);
+  Style.TMacros.push_back("blablaT");
   EXPECT_EQ(
   "blablaT(\"aa\")\n"
   "blablaT(\"aa\")\n"
@@ -10688,6 +10689,17 @@
   "  - 'CPPEVAL'\n"
   "CanonicalDelimiter: 'cc'",
   RawStringFormats, ExpectedRawStringFormats);
+
+  // FIXME: This is required because parsing a configuration simply overwrites
+  // the first N elements of the list instead of resetting it.
+  Style.TMacros.clear();
+  std::vector TestTMarco;
+  TestTMarco.push_back("_T");
+  CHECK_PARSE("TMacros: [_T]", TMacros, TestTMarco);
+  std::vector TestTMarcoAndMyT;
+  TestTMarcoAndMyT.push_back("_T");
+  TestTMarcoAndMyT.push_back("myT");
+  CHECK_PARSE("TMacros: [_T, myT]", TMacros, TestTMarcoAndMyT);
 }
 
 TEST_F(FormatTest, ParsesConfigurationWithLanguages) {
Index: lib/Format/FormatTokenLex

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated updated this revision to Diff 139687.
obfuscated added a comment.

Attached both commits in a single diff...


Repository:
  rC Clang

https://reviews.llvm.org/D44765

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7935,6 +7935,48 @@
"_T(\"Xn\"));"));
 }
 
+TEST_F(FormatTest, BreaksStringLiteralsWithin_GenericTMacro) {
+  FormatStyle Style = getLLVMStyleWithColumns(25);
+  Style.TMacros.push_back("blablaT");
+  EXPECT_EQ(
+  "blablaT(\"aa\")\n"
+  "blablaT(\"aa\")\n"
+  "blablaT(\"\")",
+  format("  blablaT(\"\")", Style));
+  EXPECT_EQ("f(x,\n"
+"  blablaT(\"\")\n"
+"  blablaT(\"aaa\"),\n"
+"  z);",
+format("f(x, blablaT(\"aaa\"), z);", Style));
+
+  // FIXME: Handle embedded spaces in one iteration.
+  //  EXPECT_EQ("blablaT(\"a\")\n"
+  //"blablaT(\"a\")\n"
+  //"blablaT(\"a\")\n"
+  //"blablaT(\"a\")",
+  //format("  blablaT ( \"\" )",
+  //   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ(
+  "blablaT ( \"\" )",
+  format("  blablaT ( \"\" )", Style));
+  EXPECT_EQ("f(\n"
+"#if !TEST\n"
+"blablaT(\"Xn\")\n"
+"#endif\n"
+");",
+format("f(\n"
+   "#if !TEST\n"
+   "blablaT(\"Xn\")\n"
+   "#endif\n"
+   ");"));
+  EXPECT_EQ("f(\n"
+"\n"
+"blablaT(\"Xn\"));",
+format("f(\n"
+   "\n"
+   "blablaT(\"Xn\"));"));
+}
+
 TEST_F(FormatTest, BreaksStringLiteralOperands) {
   // In a function call with two operands, the second can be broken with no line
   // break before it.
@@ -10647,6 +10689,17 @@
   "  - 'CPPEVAL'\n"
   "CanonicalDelimiter: 'cc'",
   RawStringFormats, ExpectedRawStringFormats);
+
+  // FIXME: This is required because parsing a configuration simply overwrites
+  // the first N elements of the list instead of resetting it.
+  Style.TMacros.clear();
+  std::vector TestTMarco;
+  TestTMarco.push_back("_T");
+  CHECK_PARSE("TMacros: [_T]", TMacros, TestTMarco);
+  std::vector TestTMarcoAndMyT;
+  TestTMarcoAndMyT.push_back("_T");
+  TestTMarcoAndMyT.push_back("myT");
+  CHECK_PARSE("TMacros: [_T, myT]", TMacros, TestTMarcoAndMyT);
 }
 
 TEST_F(FormatTest, ParsesConfigurationWithLanguages) {
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -368,7 +368,8 @@
 return false;
 
   FormatToken *Macro = Tokens[Tokens.size() - 4];
-  if (Macro->TokenText != "_T")
+  if (std::find(Style.TMacros.begin(), Style.TMacros.end(), Macro->TokenText) ==
+  Style.TMacros.end())
 return false;
 
   const char *Start = Macro->TokenText.data();
@@ -382,6 +383,7 @@
   String->TokenText, String->OriginalColumn, Style.TabWidth, Encoding);
   String->NewlinesBefore = Macro->NewlinesBefore;
   String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+  String->TMacroStringLiteral = true;
 
   Tokens.pop_back();
   Tokens.pop_back();
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -134,6 +134,9 @@
   /// 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;
 
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -430,6 +430,7 @@
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("Standard", Style.Standard);
 IO.mapOptional("TabWidth", Style.TabWidth);
+IO.mapOptional("TMacros", Style.TMacros);
 IO.mapOptional("UseTab", Style.UseTab);
   }
 };
@@ -686,6 +687,7 @@
   LLVMStyle.DisableFormat = false;
   LLVMStyle.SortIncludes = true;
   LLVMStyle.SortUsingDeclarations = true;
+  LLVMStyle.TMacro

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated marked an inline comment as done.
obfuscated added inline comments.



Comment at: lib/Format/FormatToken.h:138
+  /// \brief Whether this is a string literal similar to _T("sdfsdf").
+  bool TMacroStringLiteral = false;
+

krasimir wrote:
> 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.
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.




Comment at: unittests/Format/FormatTest.cpp:10693
+
+  // FIXME: This is required because parsing a configuration simply overwrites
+  // the first N elements of the list instead of resetting it.

krasimir wrote:
> 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 ExpectedTMacros = {"_T", "myT"};
> CHECK_PARSE("TMacros: [_T, myT]", TMacros, ExpectedTMacros);
> ```
I've copy pasted this from the foreach macro option.
I've not investigate why this fixme is there.


Repository:
  rC Clang

https://reviews.llvm.org/D44765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated updated this revision to Diff 140191.
obfuscated marked an inline comment as done.
obfuscated added a comment.

Fixed tests. Fixed description.


https://reviews.llvm.org/D44765

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7935,6 +7935,48 @@
"_T(\"Xn\"));"));
 }
 
+TEST_F(FormatTest, BreaksStringLiteralsWithin_GenericTMacro) {
+  FormatStyle Style = getLLVMStyleWithColumns(25);
+  Style.TMacros.push_back("blablaT");
+  EXPECT_EQ(
+  "blablaT(\"aa\")\n"
+  "blablaT(\"aa\")\n"
+  "blablaT(\"\")",
+  format("  blablaT(\"\")", Style));
+  EXPECT_EQ("f(x,\n"
+"  blablaT(\"\")\n"
+"  blablaT(\"aaa\"),\n"
+"  z);",
+format("f(x, blablaT(\"aaa\"), z);", Style));
+
+  // FIXME: Handle embedded spaces in one iteration.
+  //  EXPECT_EQ("blablaT(\"a\")\n"
+  //"blablaT(\"a\")\n"
+  //"blablaT(\"a\")\n"
+  //"blablaT(\"a\")",
+  //format("  blablaT ( \"\" )",
+  //   getLLVMStyleWithColumns(20)));
+  EXPECT_EQ(
+  "blablaT ( \"\" )",
+  format("  blablaT ( \"\" )", Style));
+  EXPECT_EQ("f(\n"
+"#if !TEST\n"
+"blablaT(\"Xn\")\n"
+"#endif\n"
+");",
+format("f(\n"
+   "#if !TEST\n"
+   "blablaT(\"Xn\")\n"
+   "#endif\n"
+   ");"));
+  EXPECT_EQ("f(\n"
+"\n"
+"blablaT(\"Xn\"));",
+format("f(\n"
+   "\n"
+   "blablaT(\"Xn\"));"));
+}
+
 TEST_F(FormatTest, BreaksStringLiteralOperands) {
   // In a function call with two operands, the second can be broken with no line
   // break before it.
@@ -10647,6 +10689,10 @@
   "  - 'CPPEVAL'\n"
   "CanonicalDelimiter: 'cc'",
   RawStringFormats, ExpectedRawStringFormats);
+
+  Style.TMacros.clear();
+  std::vector ExpectedTMacros = {"_T", "myT"};
+  CHECK_PARSE("TMacros: [_T, myT]", TMacros, ExpectedTMacros);
 }
 
 TEST_F(FormatTest, ParsesConfigurationWithLanguages) {
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -368,7 +368,8 @@
 return false;
 
   FormatToken *Macro = Tokens[Tokens.size() - 4];
-  if (Macro->TokenText != "_T")
+  if (std::find(Style.TMacros.begin(), Style.TMacros.end(), Macro->TokenText) ==
+  Style.TMacros.end())
 return false;
 
   const char *Start = Macro->TokenText.data();
@@ -382,6 +383,7 @@
   String->TokenText, String->OriginalColumn, Style.TabWidth, Encoding);
   String->NewlinesBefore = Macro->NewlinesBefore;
   String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+  String->TMacroStringLiteral = true;
 
   Tokens.pop_back();
   Tokens.pop_back();
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -134,6 +134,9 @@
   /// 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;
 
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -430,6 +430,7 @@
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("Standard", Style.Standard);
 IO.mapOptional("TabWidth", Style.TabWidth);
+IO.mapOptional("TMacros", Style.TMacros);
 IO.mapOptional("UseTab", Style.UseTab);
   }
 };
@@ -686,6 +687,7 @@
   LLVMStyle.DisableFormat = false;
   LLVMStyle.SortIncludes = true;
   LLVMStyle.SortUsingDeclarations = true;
+  LLVMStyle.TMacros.push_back("_T");
 
   return LLVMStyle;
 }
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1599,12 +1599,10 @@
 // FIXME: Store Prefix and Suffix (or Pre

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated added inline comments.



Comment at: lib/Format/FormatTokenLexer.cpp:386
   String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+  String->TMacroStringLiteral = true;
 

krasimir wrote:
> 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?
> 
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...


https://reviews.llvm.org/D44765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated marked 3 inline comments as done.
obfuscated added inline comments.



Comment at: lib/Format/FormatTokenLexer.cpp:371
   FormatToken *Macro = Tokens[Tokens.size() - 4];
-  if (Macro->TokenText != "_T")
+  if (std::find(Style.TMacros.begin(), Style.TMacros.end(), Macro->TokenText) 
==
+  Style.TMacros.end())

alexfh wrote:
> Consider using llvm::find, which is a range adaptor for std::find.
No idea what is range adaptor, but I can probably switch to it... :)



Comment at: lib/Format/FormatTokenLexer.cpp:386
   String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+  String->TMacroStringLiteral = true;
 

alexfh wrote:
> obfuscated wrote:
> > krasimir wrote:
> > > 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?
> > > 
> > 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...
> > 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).
>> 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?





https://reviews.llvm.org/D44765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Teodor Petrov via Phabricator via cfe-commits]

obfuscated marked an inline comment as done.
obfuscated added a comment.

Ping?


https://reviews.llvm.org/D44765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits