clang-format: Add ControlStatementsExceptForEachMacros option to SpaceBeforeParens

[DaanDeMeyer]

Summary

systemd recently added a clang-format file. One issue I encountered in using clang-format on systemd is that systemd does not add a space before the parens of their foreach macros but clang-format always adds a space. This does not seem to be configurable in clang-format. This revision adds the ControlStatementsExceptForEachMacros option to SpaceBeforeParens which puts a space before all control statement parens except ForEach macros. This drastically reduces the amount of changes when running clang-format on systemd's source code.

[DaanDeMeyer]

Fixed formatting

[MyDeveloperDay]

Could you add to the review description a link to the systemd style guide that says it should be like this?

clang/docs/ClangFormatStyleOptions.rst

2311

Normally this would be generated from the Format.h after running the dump style script in tools, did you?

clang/include/clang/Format/Format.h

1958

can you move this down below this comment which applies to SBPO_ControlStatements and add your own comment explaining why you might NOT want them in front of the ForeachEach Macros which I have to say I'm confused why you wouldn't but then its not for me to understand you style requirements.

[DaanDeMeyer]

• Moved docs to Format.h and re-generated rst file using dump_format_style.py
• Expanded comment with an explanation for why you might want to not have a space before the parens of a ForEach macro

This isn't explicitly mentioned in systemd's style guide. I simply did a regex search over systemd's codebase. Searching (using regex) for ForEach macro usages with a space before the parens returns 7 matches over the entire codebase. Searching for ForEach macro usages without a space before the parens returns 1753 matches. If needed, I can make a systemd PR that proposes explicitly adding this to the style guide.

I only added the relevant changes produced by dump_format_style to this revision but there were some other changes produced as well (just to let you know).

[DaanDeMeyer]

clang/docs/ClangFormatStyleOptions.rst

2311

I didn't think the docs were automatically generated. I added a comment to Format.h and re-generated the docs using the dump_format_style script.

[MyDeveloperDay]

I've added a couple of others reviewers to comment, I don't personally mind as I don't use the ForEach Macros. It feels kind of funny to me to differentiate just the for macros from for,if,while,for.. (I guess I'm struggling to understand why you wouldn't just add the space other than the churn you'll get)

But I won't block it if others are ok with it.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

982

UNKNOWN_FOREACH?

[MyDeveloperDay]

If there is no additional comments from others then I'm fine giving this a LGTM, if it means we can encourage other big projects to go full clang-format

[DaanDeMeyer]

(I don't have commit access so it'd be great if someone could merge this for me, at least that's how my other contributions have been handled)

[MyDeveloperDay]

In D78869#2019933, @DaanDeMeyer wrote:

(I don't have commit access so it'd be great if someone could merge this for me, at least that's how my other contributions have been handled)

You really should think about getting commit access especially as this isn't your first patch, I found it the best way to gain experience and its nice to have new people on board with us.

[DaanDeMeyer]

I should have commit access now. Is there any documentation on how exactly to do a merge? Do I just add the commit to master and push? I apologize if this seems obvious, I just want to make sure I'm not inadvertently breaking something.

[mitchell-stellar]

I often:

1. Commit the change to my local repo, following the style of previous commit messages for clang-format.
2. Build and run the clang-format tests and make sure they pass.
3. Pull, rebase, and push. (Linear history is enforced.)
4. Build and run the clang-format tests again just in case. (Optional)

[MyDeveloperDay]

In D78869#2022850, @mitchell-stellar wrote:

I often:

I develop and run on windows (using a cygwin shell), but with visual studio compiler (configured with CMake for ninja)

0.5  after doing git add of the files, I do `git clang-format` just to be sure  (adding any files that are modified)

1. Commit the change to my local repo, following the style of previous commit messages for clang-format.

So I have a script which gets the commit message

get_commit_message.sh
----
echo '{ "revision_id": '${1:1}' }' | arc call-conduit --conduit-uri https://reviews.llvm.org/ --conduit-token <phabricator-token> differential.getcommitmessage | /usr/bin/jq -r ".response"
---

This will fetch the commit message from Phabricator that I should use. (you need to get a phabricator-token from your profile->settings)

git fetch  && git pull --rebase --autostash

2. Build and run the clang-format tests and make sure they pass.
3. Pull, rebase, and push. (Linear history is enforced.)
4. Build and run the clang-format tests again just in case. (Optional)

5. I try to run the lit tests too. (in clang/tests/Format)
6. I check the documentation if any rst files are changed ( /usr/bin/sphinx-build -n ./docs ./html)
   
   // assuming no new changes.. git push

[DaanDeMeyer]

I pushed the commit to github. Thanks for the help!

clang/docs/ClangFormatStyleOptions.rst

* ``SBPO_ControlStatements`` (in configuration: ``ControlStatements``)
     .. code-block:: c++
 
        void f() {
          if (true) {
            f();
          }
        }
 
+  * ``SBPO_ControlStatementsExceptForEachMacros`` (in configuration: 
+    ``ControlStatementsExceptForEachMacros``)
+    Same as ``SBPO_ControlStatements`` except this option doesn't apply to
+    ForEach macros.
+
+    .. code-block:: c++
+
+       void f() {
+         Q_FOREACH(...) {
+           f();
+         }
+       }
+
   * ``SBPO_NonEmptyParentheses`` (in configuration: ``NonEmptyParentheses``)
     Put a space before opening parentheses only if the parentheses are not
     empty i.e. '()'
 
     .. code-block:: c++
 
       void() {
         if (true) {

clang/include/clang/Format/Format.h

enum SpaceBeforeParensOptions {
     /// \code
     ///    void f() {
     ///      if (true) {
     ///        f();
     ///      }
     ///    }
     /// \endcode
     SBPO_ControlStatements,
+    SBPO_ControlStatementsExceptForEachMacros,
     /// Put a space before opening parentheses only if the parentheses are not
     /// empty i.e. '()'
     /// \code
     ///   void() {
     ///     if (true) {
     ///       f();
     ///       g (x, y, z);
     ///     }
     ///   }

clang/lib/Format/Format.cpp

 
 template <>
 struct ScalarEnumerationTraits<FormatStyle::SpaceBeforeParensOptions> {
   static void enumeration(IO &IO,
                           FormatStyle::SpaceBeforeParensOptions &Value) {
     IO.enumCase(Value, "Never", FormatStyle::SBPO_Never);
     IO.enumCase(Value, "ControlStatements",
                 FormatStyle::SBPO_ControlStatements);
+    IO.enumCase(Value, "ControlStatementsExceptForEachMacros",
+                FormatStyle::SBPO_ControlStatementsExceptForEachMacros);
     IO.enumCase(Value, "NonEmptyParentheses",
                 FormatStyle::SBPO_NonEmptyParentheses);
     IO.enumCase(Value, "Always", FormatStyle::SBPO_Always);
 
     // For backward compatibility.
     IO.enumCase(Value, "false", FormatStyle::SBPO_Never);
     IO.enumCase(Value, "true", FormatStyle::SBPO_ControlStatements);
   }

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   if (Left.is(TT_BlockComment))
     // No whitespace in x(/*foo=*/1), except for JavaScript.
     return Style.Language == FormatStyle::LK_JavaScript ||
            !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
     if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
         (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
       return true;
+    if (Style.SpaceBeforeParens ==
+            FormatStyle::SBPO_ControlStatementsExceptForEachMacros &&
+        Left.is(TT_ForEachMacro))
+      return false;
     return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
            (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
             (Left.isOneOf(tok::pp_elif, tok::kw_for, tok::kw_while,
                           tok::kw_switch, tok::kw_case, TT_ForEachMacro,
                           TT_ObjCForIn) ||
              Left.isIf(Line.Type != LT_PreprocessorDirective) ||
              (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
                            tok::kw_new, tok::kw_delete) &&

clang/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, ForEachLoops) {
   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"
                "}");
 
+  FormatStyle Style = getLLVMStyle();
+  Style.SpaceBeforeParens =
+      FormatStyle::SBPO_ControlStatementsExceptForEachMacros;
+  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"
+               "}",
+               Style);
+
   // As function-like macros.
   verifyFormat("#define foreach(x, y)\n"
                "#define Q_FOREACH(x, y)\n"
                "#define BOOST_FOREACH(x, y)\n"
                "#define UNKNOWN_FOREACH(x, y)\n");
 
   // Not as function-like macros.
   verifyFormat("#define foreach (x, y)\n"