[PATCH] When in pre-c++11 mode, treat _T("xxx") as a single string literal, repeat the _T() part around each fragment. This addresses http://llvm.org/PR17122

Alexander Kornienko alexfh at google.com
Fri Sep 13 05:40:07 PDT 2013



================
Comment at: lib/Format/ContinuationIndenter.cpp:679
@@ +678,3 @@
+    if ((Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")")) ||
+        (Text.endswith(Postfix = "\"") && (Text.startswith(Prefix = "\"") ||
+                                          Text.startswith(Prefix = "u\"") ||
----------------
Daniel Jasper wrote:
> Who came up with this format?
> 
> Also these seem like a lot of string comparisons (albeit short strings). Would it make sense to precalculate this to get it out of the critical path?
Clang-format produced this format before I added parens around the second operand of ||.

Fixed.

================
Comment at: lib/Format/Format.cpp:602
@@ -600,1 +601,3 @@
 private:
+  void maybeJoinLastTokens() {
+    size_t Last = Tokens.size() - 1;
----------------
Daniel Jasper wrote:
> I find "last" slightly ambiguous (could also be the last in the line, the file, ..). How about maybeJoinPreviousTokens()?
Ok.

================
Comment at: lib/Format/Format.cpp:604
@@ +603,3 @@
+    size_t Last = Tokens.size() - 1;
+    if (Style.Standard != format::FormatStyle::LS_Cpp11 && Last > 2 &&
+        Tokens[Last]->is(tok::r_paren) &&
----------------
Daniel Jasper wrote:
> I think this would be slightly easier to understand (and possibly extend) if written like:
> 
>   size_t Last = Tokens.size() - 1; // Or LastIndex or something...
>   if (Last < 2 || Tokens.back()->isNot(tok::r_paren))
>     return;
> 
>   if (Tokens[Last - 1]->isNot(tok::string_literal) ||
>       Tokens[Last - 1]->IsMultiline)
>     return;
>   FormatToken *String = Tokens[Last - 1];
> 
>   if (Tokens[Last - 2]->isNot(tok::l_paren) ||
>       Tokens[Last - 3]->TokenText != "_T")
>     return;
>   FormatToken *Macro = Tokens[Last - 3];
> 
>   ...
Not that I like it more, but if you prefer this layout, I've implemented a variant of it.

================
Comment at: unittests/Format/FormatTest.cpp:5501
@@ +5500,3 @@
+
+// FIXME: Handle embedded spaces in one iteration.
+//  EXPECT_EQ("_T(\"aaaaaaaaaaaaa\")\n"
----------------
Daniel Jasper wrote:
> Indent
Done.


http://llvm-reviews.chandlerc.com/D1640



More information about the cfe-commits mailing list