clang-format: Fix space in for-each macro definition.

[strager]

Summary

The C preprocessor definition of a for-each macro was being
improperly formatted. A space was being inserted between the
macro name and the opening parenthesis. Fix the extra space
by excluding such cases from the ForEachMacros checks.

[djasper]

Please use unittests/Format/FormatTest.cpp for the unit test.

lib/Format/Format.cpp

1130–1131

Generally use clang-format. Also, why are you using two nested if conditions instead of just merging this into the other one?

[strager]

Please use unittests/Format/FormatTest.cpp for the unit test.

Okay. (Should the other tests be ported too? Is there something special about the tests currently in test/Format?)

lib/Format/Format.cpp

1130–1131

I'll merge them.

[djasper]

The fundamental difference is that the tests in test/Format verify the behavior of the clang-format binary, e.g. with regard to certain command line flags, etc.

You are testing a specific formatting behavior (of the clang-format library), which is done in unittests/Format.

[strager]

Move tests into FormatTest.

[strager]

I got this message from arc diff:

Invalid Content Encoding (Non-UTF8)
This diff includes a file which is not valid UTF-8 (it has invalid byte
sequences). You can either stop this workflow and fix it, or continue. If you
continue, this file will be marked as binary.

You can learn more about how Phabricator handles character encodings (and how
to configure encoding settings and detect and correct encoding problems) by
reading 'User Guide: UTF-8 and Character Encoding' in the Phabricator
documentation.

    AFFECTED FILE
    unittests/Format/FormatTest.cpp

    Do you want to mark this file as binary and continue? [Y/n]

That's why unittests/Format/FormatTest.cpp is marked as binary. *sigh*

Here's the git-diff:

diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp
index 69d6cee..0c45f3b 100644
--- a/unittests/Format/FormatTest.cpp
+++ b/unittests/Format/FormatTest.cpp
@@ -610,12 +610,20 @@ TEST_F(FormatTest, RangeBasedForLoops) {
 }

 TEST_F(FormatTest, ForEachLoops) {
+  verifyFormat("#define foreach(x, y)\n"
+               "#define Q_FOREACH(x, y)\n"
+               "#define BOOST_FOREACH(x, y)\n"
+               "#define UNKNOWN_FOREACH(x, y)\n");
   verifyFormat("void f() {\n"
                "  foreach (Item *item, itemlist) {}\n"
                "  Q_FOREACH (Item *item, itemlist) {}\n"
                "  BOOST_FOREACH (Item *item, itemlist) {}\n"
                "  UNKNOWN_FORACH(Item * item, itemlist) {}\n"
                "}");
+  verifyFormat("#define foreach (x, y)\n"
+               "#define Q_FOREACH (x, y)\n"
+               "#define BOOST_FOREACH (x, y)\n"
+               "#define UNKNOWN_FOREACH (x, y)\n");
 }

 TEST_F(FormatTest, FormatsWhileLoop) {

[djasper]

The patch looks good and I have submitted it as r239513 (didn't know whether you have commit access).

However, we cannot reproduce FormatTest.cpp being treated as binary. We have tried with git and svn and looked at whether there might be something wrong with phabricator, but everything seems fine. Can you try to create proper patches for all of you other changes? If it doesn't work with ARC, can you just use the Web interface to upload the patches (ideally created with full-file context)? Thanks.

[strager]

I understand why you cannot reproduce the arc diff problem. Facebook has a(n unpublished) change in arc which blocks non-BMP code points from being uploaded in a diff. (I'm using Facebook's arc.) FormatTest.cpp contains Japanese characters, which lie outside the BMP.

This isn't a problem with LLVM's Phabricator setup; it's a problem with my setup.

lib/Format/Format.cpp

if (FirstNewlinePos == StringRef::npos) {
       // The last line of the token always starts in column 0.
       // Thus, the length can be precomputed even in the presence of tabs.
       FormatTok->LastLineColumnWidth = encoding::columnWidthWithTabs(
           Text.substr(Text.find_last_of('\n') + 1), 0, Style.TabWidth,
           Encoding);
       Column = FormatTok->LastLineColumnWidth;
     }
 
-    if (std::find(ForEachMacros.begin(), ForEachMacros.end(),
+    if (!(Tokens.size() > 0 && Tokens.back()->Tok.getIdentifierInfo() &&
+          Tokens.back()->Tok.getIdentifierInfo()->getPPKeywordID() ==
+              tok::pp_define) &&
+        std::find(ForEachMacros.begin(), ForEachMacros.end(),
                   FormatTok->Tok.getIdentifierInfo()) != ForEachMacros.end())
       FormatTok->Type = TT_ForEachMacro;
 
     return FormatTok;
   }
 
   FormatToken *FormatTok;
   bool IsFirstToken;
   bool GreaterStashed, LessStashed;
   unsigned Column;
   unsigned TrailingWhitespace;