This is an archive of the discontinued LLVM Phabricator instance.

clang-format: Add ControlStatementsExceptForEachMacros option to SpaceBeforeParens
ClosedPublic

Authored by DaanDeMeyer on Apr 25 2020, 12:43 PM.

Details

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.

Diff Detail

Event Timeline

DaanDeMeyer created this revision.Apr 25 2020, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2020, 12:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko edited reviewers, added: MyDeveloperDay; removed: Restricted Project.Apr 25 2020, 3:15 PM
Eugene.Zelenko removed a subscriber: Restricted Project.

Fixed formatting

MyDeveloperDay added a comment.EditedApr 26 2020, 4:24 AM

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
2339

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
1997

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.

  • 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 marked 3 inline comments as done.Apr 26 2020, 5:19 AM
DaanDeMeyer added inline comments.
clang/docs/ClangFormatStyleOptions.rst
2339

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.

DaanDeMeyer marked an inline comment as done.Apr 26 2020, 5:19 AM

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 added inline comments.Apr 26 2020, 9:36 AM
clang/unittests/Format/FormatTest.cpp
982

UNKNOWN_FOREACH?

MyDeveloperDay accepted this revision.May 5 2020, 3:14 AM

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

This revision is now accepted and ready to land.May 5 2020, 3:14 AM

(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)

(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.

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.

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)

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
  1. Build and run the clang-format tests and make sure they pass.
  2. Pull, rebase, and push. (Linear history is enforced.)
  3. Build and run the clang-format tests again just in case. (Optional)
  1. I try to run the lit tests too. (in clang/tests/Format)
  2. I check the documentation if any rst files are changed ( /usr/bin/sphinx-build -n ./docs ./html)

    // assuming no new changes.. git push

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

This revision was automatically updated to reflect the committed changes.