Add support for foreach macros to clang-format

[briangreenery]

Summary

I added a new configuration option to clang-format called "ForEachMacros". This
allows macros that work like a foreach loop, like BOOST_FOREACH, to be formatted
correctly. Currently they're formatted like a function call.

This would fix:

http://llvm.org/bugs/show_bug.cgi?id=18534

Which these bugs could then be duped on:

http://llvm.org/bugs/show_bug.cgi?id=17518
http://llvm.org/bugs/show_bug.cgi?id=17242

[majnemer]

Would be neat if the option could take regular expressions. The linux kernel contains many foreach macros and it would be nice to concisely match them. :)

lib/Format/FormatToken.cpp

67 ↗

(On Diff #7460)

Can't this be std::find(std::begin(Style.ForEachMacros), std::end(Style.ForEachMacros), Name) != std::end(Style.ForEachMacros); ?

[briangreenery]

Change ForEachMacros to be a regex instead of a list of strings

Using a regex instead of a list of strings sounds like a much better idea.

I also added another new configuration option SBPO_ControlStatementsAndForEachMacros since it seems like some people put spaces after foreach macros (like the example in bug 17242, bug 18534, and the code I normally work on), and some people don't (like the linux kernel).

[briangreenery]

Is there something I can do to help get this reviewed?

[djasper]

I don't think using regular expressions is the right way to go here:

• We'll want to precisely control which macros are a "foreach". Otherwise, it is too likely to accidentally come up with a method name that matches the regex.
• Regular expressions have a performance impact, especially in the way they are used here (not precomputed once per token).

I would argue for making this a (e.g. comma-separated) list of macros, which gets parsed into the identifier table used to lookup most of the other identifiers.

docs/ClangFormatStyleOptions.rst

311

Do we really want to treat those separate from control statements? I don't see a good/valid reason for doing so...

[majnemer]

@djasper, there are over 500 such macros in the linux kernel. I think a regex is more appropriate than a complete list of such macros.

[djasper]

1. I don't.
2. How would you verify that your regex addresses all of those 500 macros.
3. If you are loose enough, how do you guarantee that you don't also include a function that is not a foreach macro?
4. Are there really 500 different foreach macros? I couldn't come up with that many different ways of spelling it.

[majnemer]

Here is a small subset of such macros to give you an example:
list_for_each_cookie
list_for_each_entry
list_for_each_entry_continue
list_for_each_entry_continue_rcu
list_for_each_entry_continue_reverse
list_for_each_entry_from
list_for_each_entry_rcu
list_for_each_entry_reverse
list_for_each_entry_safe
list_for_each_entry_safe_continue
list_for_each_entry_safe_from
list_for_each_entry_safe_reverse
list_for_each_from
list_for_each_prev
list_for_each_prev_safe
list_for_each_safe
list_for_each_xattr
radix_tree_for_each_chunk
radix_tree_for_each_chunk_slot
radix_tree_for_each_contig
radix_tree_for_each_slot
radix_tree_for_each_tagged

I used the following to find these:
git grep for_each | grep define | grep -P "\w+_for_each\w+" -o | sort -u

That macro gave me no false positives.

[djasper]

• Not all of those qualify for what this CL implements. They don't expect a variable declaration as the first parameter and don't expect users to add a space between macro name and opening parenthesis. These are false positives (I think actually almost all of the ones you are mentioning).
• The regex has false positives, e.g. in the git codebase, which defines functions with such names.

[briangreenery]

I tried to go with the approach that djasper outlined: support a static list of macros parsed into an identifier table used for lookup.

I'm sorry for the delay in updating this.

[djasper]

A few comments, but otherwise this looks good.

Thank you for working on this. Do you have commit access?

lib/Format/UnwrappedLineParser.cpp

663

Don't fall through implicitly, add "break;".

1051

Nit: 'for', 'while' or foreach macro expected.

lib/Format/UnwrappedLineParser.h

88

I don't see any reason to rename this. ForEachLoops are for loops (which also include range-based for loops).

[briangreenery]

Okay, I made those changes. I don't have commit access.

[briangreenery]

It looks like phabricator only picked up one of the changes to FormatTest.cpp. Here's the diff generated by 'svn diff', which has all changes in it.{F51395}

[djasper]

Added basic testing and minor tweaks and submitted as r205307.

Thank you!

docs/ClangFormatStyleOptions.rst

**ExperimentalAutoDetectBinPacking** (``bool``)
   Each call can be bin-packed, one-per-line or inconclusive. If it is
   inconclusive, e.g. completely on one line, but a decision needs to be
   made, clang-format analyzes whether there are other bin-packed cases in
   the input file and act accordingly.
 
   NOTE: This is an experimental flag, that might go away or be renamed. Do
   not use this in config files, etc. Use at your own risk.
 
+**ForEachMacros** (``std::vector<std::string>``)
+  A list of macros that should be interpreted as foreach loops instead of as
+  function calls.
+
+  For example, ``ForEachMacros: [BOOST_FOREACH, Q_FOREACH]`` tells
+  clang-format to treat ``BOOST_FOREACH`` and ``Q_FOREACH`` as loop control
+  statements.
+
 **IndentCaseLabels** (``bool``)
   Indent case labels one level from the switch statement.
 
   When ``false``, use the same indentation level as for the switch statement.
   Switch statement body is always indented one level more than case labels.
 
 **IndentFunctionDeclarationAfterType** (``bool``)
   If ``true``, indent when breaking function declarations which
**SpaceBeforeParens** (``SpaceBeforeParensOptions``)
 
   Possible values:
 
   * ``SBPO_Never`` (in configuration: ``Never``)
     Never put a space before opening parentheses.
   * ``SBPO_ControlStatements`` (in configuration: ``ControlStatements``)
     Put a space before opening parentheses only after control statement
     keywords (``for/if/while...``).
   * ``SBPO_Always`` (in configuration: ``Always``)
     Always put a space before opening parentheses, except when it's
     prohibited by the syntax rules (in function-like macro definitions) or
     when determined by other style rules (after unary operators, opening
     parentheses, etc.)
 
 
 **SpaceInEmptyParentheses** (``bool``)
   If ``true``, spaces may be inserted into '()'.

include/clang/Format/Format.h

struct FormatStyle {
 
   /// \brief Indent width for line continuations.
   unsigned ContinuationIndentWidth;
 
   /// \brief A regular expression that describes comments with special meaning,
   /// which should not be split into lines or otherwise changed.
   std::string CommentPragmas;
 
+  /// \brief An array of macros that should be interpreted as foreach loops
+  /// instead of as function calls.
+  ///
+  /// These are expected to be macros of the form:
+  /// \code
+  /// FOREACH(<variable-declaration>, ...)
+  ///   <loop-body>
+  /// \endcode
+  ///
+  /// For example: BOOST_FOREACH.
+  std::vector<std::string> ForEachMacros;
+
   bool operator==(const FormatStyle &R) const {
     return AccessModifierOffset == R.AccessModifierOffset &&
            ConstructorInitializerIndentWidth ==
                R.ConstructorInitializerIndentWidth &&
            AlignEscapedNewlinesLeft == R.AlignEscapedNewlinesLeft &&
            AlignTrailingComments == R.AlignTrailingComments &&
            AllowAllParametersOfDeclarationOnNextLine ==
                R.AllowAllParametersOfDeclarationOnNextLine &&
return AccessModifierOffset == R.AccessModifierOffset &&
            UseTab == R.UseTab && SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInAngles == R.SpacesInAngles &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&
            SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
            SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
            ContinuationIndentWidth == R.ContinuationIndentWidth &&
-           CommentPragmas == R.CommentPragmas;
+           CommentPragmas == R.CommentPragmas &&
+           ForEachMacros == R.ForEachMacros;
   }
 };
 
 /// \brief Returns a format style complying with the LLVM coding standards:
 /// http://llvm.org/docs/CodingStandards.html.
 FormatStyle getLLVMStyle();
 
 /// \brief Returns a format style complying with one of Google's style guides:

lib/Format/Format.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/YAMLTraits.h"
 #include <queue>
 #include <string>
 
 using clang::format::FormatStyle;
 
+LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(std::string)
+
 namespace llvm {
 namespace yaml {
 template <> struct ScalarEnumerationTraits<FormatStyle::LanguageKind> {
   static void enumeration(IO &IO, FormatStyle::LanguageKind &Value) {
     IO.enumCase(Value, "Cpp", FormatStyle::LK_Cpp);
     IO.enumCase(Value, "JavaScript", FormatStyle::LK_JavaScript);
     IO.enumCase(Value, "Proto", FormatStyle::LK_Proto);
   }
static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("SpacesInCStyleCastParentheses",
                    Style.SpacesInCStyleCastParentheses);
     IO.mapOptional("SpacesInContainerLiterals",
                    Style.SpacesInContainerLiterals);
     IO.mapOptional("SpaceBeforeAssignmentOperators",
                    Style.SpaceBeforeAssignmentOperators);
     IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
     IO.mapOptional("CommentPragmas", Style.CommentPragmas);
+    IO.mapOptional("ForEachMacros", Style.ForEachMacros); 
 
     // For backward compatibility.
     if (!IO.outputting()) {
       IO.mapOptional("SpaceAfterControlStatementKeyword",
                      Style.SpaceBeforeParens);
     }
     IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
   }

 class FormatTokenLexer {
 public:
   FormatTokenLexer(Lexer &Lex, SourceManager &SourceMgr, FormatStyle &Style,
                    encoding::Encoding Encoding)
       : FormatTok(NULL), IsFirstToken(true), GreaterStashed(false), Column(0),
         TrailingWhitespace(0), Lex(Lex), SourceMgr(SourceMgr), Style(Style),
         IdentTable(getFormattingLangOpts()), Encoding(Encoding) {
     Lex.SetKeepWhitespaceMode(true);
+
+    for (auto I : Style.ForEachMacros)
+      ForEachMacros.push_back(&IdentTable.get(I));
+    std::sort(ForEachMacros.begin(), ForEachMacros.end());
   }
 
   ArrayRef<FormatToken *> lex() {
     assert(Tokens.empty());
     do {
       Tokens.push_back(getNextToken());
       tryMergePreviousTokens();
     } while (Tokens.back()->Tok.isNot(tok::eof));
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;
     }
 
+    FormatTok->IsForEachMacro =
+        std::binary_search(ForEachMacros.begin(), ForEachMacros.end(),
+                           FormatTok->Tok.getIdentifierInfo());
+
     return FormatTok;
   }
 
   FormatToken *FormatTok;
   bool IsFirstToken;
   bool GreaterStashed;
   unsigned Column;
   unsigned TrailingWhitespace;
   Lexer &Lex;
   SourceManager &SourceMgr;
   FormatStyle &Style;
   IdentifierTable IdentTable;
   encoding::Encoding Encoding;
   llvm::SpecificBumpPtrAllocator<FormatToken> Allocator;
   SmallVector<FormatToken *, 16> Tokens;
+  SmallVector<IdentifierInfo*, 8> ForEachMacros;
 
   void readRawToken(FormatToken &Tok) {
     Lex.LexFromRawLexer(Tok.Tok);
     Tok.TokenText = StringRef(SourceMgr.getCharacterData(Tok.Tok.getLocation()),
                               Tok.Tok.getLength());
     // For formatting, treat unterminated string literals like normal string
     // literals.
     if (Tok.is(tok::unknown)) {

lib/Format/FormatToken.h

FormatToken()
         IsFirst(false), MustBreakBefore(false), IsUnterminatedLiteral(false),
         BlockKind(BK_Unknown), Type(TT_Unknown), SpacesRequiredBefore(0),
         CanBreakBefore(false), ClosesTemplateDeclaration(false),
         ParameterCount(0), PackingKind(PPK_Inconclusive), TotalLength(0),
         UnbreakableTailLength(0), BindingStrength(0), NestingLevel(0),
         SplitPenalty(0), LongestObjCSelectorName(0), FakeRParens(0),
         StartsBinaryExpression(false), EndsBinaryExpression(false),
         LastInChainOfCalls(false), PartOfMultiVariableDeclStmt(false),
-        MatchingParen(NULL), Previous(NULL), Next(NULL),
+        IsForEachMacro(false), MatchingParen(NULL), Previous(NULL), Next(NULL),
         Decision(FD_Unformatted), Finalized(false) {}
 
   /// \brief The \c Token.
   Token Tok;
 
   /// \brief The number of newlines immediately before the \c Token.
   ///
   /// This can be used to determine what the user wrote in the original code
struct FormatToken {
   /// \brief Is this the last "." or "->" in a builder-type call?
   bool LastInChainOfCalls;
 
   /// \brief Is this token part of a \c DeclStmt defining multiple variables?
   ///
   /// Only set if \c Type == \c TT_StartOfName.
   bool PartOfMultiVariableDeclStmt;
 
+  /// \brief Is this a for each macro?
+  bool IsForEachMacro;
+
   bool is(tok::TokenKind Kind) const { return Tok.is(Kind); }
 
   bool isOneOf(tok::TokenKind K1, tok::TokenKind K2) const {
     return is(K1) || is(K2);
   }
 
   bool isOneOf(tok::TokenKind K1, tok::TokenKind K2, tok::TokenKind K3) const {
     return is(K1) || is(K2) || is(K3);

lib/Format/TokenAnnotator.cpp

if (Left->Previous &&
                Left->Previous->MatchingParen->Type == TT_LambdaLSquare) {
       // This is a parameter list of a lambda expression.
       Contexts.back().IsExpression = false;
     } else if (Contexts[Contexts.size() - 2].CaretFound) {
       // This is the parameter list of an ObjC block.
       Contexts.back().IsExpression = false;
     } else if (Left->Previous && Left->Previous->is(tok::kw___attribute)) {
       Left->Type = TT_AttributeParen;
+    } else if (Left->Previous && Left->Previous->IsForEachMacro) {
+      // The first argument to a foreach macro is a declaration.
+      Contexts.back().IsForEachMacro = true;
+      Contexts.back().IsExpression = false;
     }
 
     if (StartsObjCMethodExpr) {
       Contexts.back().ColonIsObjCMethodExpr = true;
       Left->Type = TT_ObjCMethodExpr;
     }
 
     bool MightBeFunctionType = CurrentToken->is(tok::star);
case tok::identifier:
           Tok->Tok.getIdentifierInfo() == &Ident_in)
         Tok->Type = TT_ObjCForIn;
       break;
     case tok::comma:
       if (Contexts.back().FirstStartOfName)
         Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
       if (Contexts.back().InCtorInitializer)
         Tok->Type = TT_CtorInitializerComma;
+      if (Contexts.back().IsForEachMacro)
+        Contexts.back().IsExpression = true;
       break;
     default:
       break;
     }
     return true;
   }
 
   void parseIncludeDirective() {
struct Context {
     Context(tok::TokenKind ContextKind, unsigned BindingStrength,
             bool IsExpression)
         : ContextKind(ContextKind), BindingStrength(BindingStrength),
           LongestObjCSelectorName(0), NumBlockParameters(0),
           ColonIsForRangeExpr(false), ColonIsDictLiteral(false),
           ColonIsObjCMethodExpr(false), FirstObjCSelectorName(NULL),
           FirstStartOfName(NULL), IsExpression(IsExpression),
           CanBeExpression(true), InTemplateArgument(false),
-          InCtorInitializer(false), CaretFound(false) {}
+          InCtorInitializer(false), CaretFound(false), IsForEachMacro(false) {}
 
     tok::TokenKind ContextKind;
     unsigned BindingStrength;
     unsigned LongestObjCSelectorName;
     unsigned NumBlockParameters;
     bool ColonIsForRangeExpr;
     bool ColonIsDictLiteral;
     bool ColonIsObjCMethodExpr;
     FormatToken *FirstObjCSelectorName;
     FormatToken *FirstStartOfName;
     bool IsExpression;
     bool CanBeExpression;
     bool InTemplateArgument;
     bool InCtorInitializer;
     bool CaretFound;
+    bool IsForEachMacro;
   };
 
   /// \brief Puts a new \c Context onto the stack \c Contexts for the lifetime
   /// of each instance.
   struct ScopedContextCreator {
     AnnotatingParser &P;
 
     ScopedContextCreator(AnnotatingParser &P, tok::TokenKind ContextKind,
if (Left.is(tok::colon))
     return Left.Type != TT_ObjCMethodExpr;
   if (Right.is(tok::l_paren)) {
     if (Left.is(tok::r_paren) && Left.Type == TT_AttributeParen)
       return true;
     return Line.Type == LT_ObjCDecl ||
            Left.isOneOf(tok::kw_return, tok::kw_new, tok::kw_delete,
                         tok::semi) ||
            (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-            Left.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
-                         tok::kw_catch)) ||
+            (Left.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while,
+                          tok::kw_switch, tok::kw_catch) ||
+             Left.IsForEachMacro)) ||
            (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
             Left.isOneOf(tok::identifier, tok::kw___attribute) &&
             Line.Type != LT_PreprocessorDirective);
   }
   if (Left.is(tok::at) && Right.Tok.getObjCKeywordID() != tok::objc_not_keyword)
     return false;
   if (Left.is(tok::l_brace) && Right.is(tok::r_brace))
     return !Left.Children.empty(); // No spaces in "{}".

lib/Format/UnwrappedLineParser.h

private:
   void parsePPEndIf();
   void parsePPUnknown();
   void parseStructuralElement();
   bool tryToParseBracedList();
   bool parseBracedList(bool ContinueOnSemicolons = false);
   void parseParens();
   void parseSquare();
   void parseIfThenElse();
-  void parseForOrWhileLoop();
+  void parseForOrWhileOrForEachLoop();
   void parseDoWhile();
   void parseLabel();
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
   void parseAccessSpecifier();
   void parseEnum();
   void parseRecord();

lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseStructuralElement() {
   case tok::kw_private:
     parseAccessSpecifier();
     return;
   case tok::kw_if:
     parseIfThenElse();
     return;
   case tok::kw_for:
   case tok::kw_while:
-    parseForOrWhileLoop();
+    parseForOrWhileOrForEachLoop();
     return;
   case tok::kw_do:
     parseDoWhile();
     return;
   case tok::kw_switch:
     parseSwitch();
     return;
   case tok::kw_default:
if (FormatTok->Tok.is(tok::string_literal)) {
       if (FormatTok->Tok.is(tok::l_brace)) {
         parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
         addUnwrappedLine();
         return;
       }
     }
     // In all other cases, parse the declaration.
     break;
+  case tok::identifier:
+    if (FormatTok->IsForEachMacro) {
+      parseForOrWhileOrForEachLoop();
+      return;
+    }
   default:
     break;
   }
   do {
     switch (FormatTok->Tok.getKind()) {
     case tok::at:
       nextToken();
       if (FormatTok->Tok.is(tok::l_brace))
if (FormatTok->Tok.is(tok::l_brace)) {
     // think. Puttin the semicolon into its own line is very ugly.
     if (FormatTok->Tok.is(tok::semi))
       nextToken();
     addUnwrappedLine();
   }
   // FIXME: Add error handling.
 }
 
-void UnwrappedLineParser::parseForOrWhileLoop() {
-  assert((FormatTok->Tok.is(tok::kw_for) || FormatTok->Tok.is(tok::kw_while)) &&
-         "'for' or 'while' expected");
+void UnwrappedLineParser::parseForOrWhileOrForEachLoop() {
+  assert((FormatTok->Tok.is(tok::kw_for) || FormatTok->Tok.is(tok::kw_while) ||
+          FormatTok->IsForEachMacro) &&
+         "'for' or 'while' or foreach macro expected");
   nextToken();
   if (FormatTok->Tok.is(tok::l_paren))
     parseParens();
   if (FormatTok->Tok.is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     addUnwrappedLine();
   } else {

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfiguration) {
 
   Style.NamespaceIndentation = FormatStyle::NI_All;
   CHECK_PARSE("NamespaceIndentation: None", NamespaceIndentation,
               FormatStyle::NI_None);
   CHECK_PARSE("NamespaceIndentation: Inner", NamespaceIndentation,
               FormatStyle::NI_Inner);
   CHECK_PARSE("NamespaceIndentation: All", NamespaceIndentation,
               FormatStyle::NI_All);
+
+  std::vector<std::string> BoostForeach = { "BOOST_FOREACH" };
+  CHECK_PARSE("ForEachMacros: [BOOST_FOREACH]", ForEachMacros, BoostForeach);
+  std::vector<std::string> BoostAndQForeach = { "BOOST_FOREACH", "Q_FOREACH" };
+  CHECK_PARSE("ForEachMacros: [BOOST_FOREACH, Q_FOREACH]", ForEachMacros,
+              BoostAndQForeach);
 }
 
 TEST_F(FormatTest, ParsesConfigurationWithLanguages) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE("Language: Cpp\n"
               "IndentWidth: 12",
               IndentWidth, 12u);