Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang-tidy readability-braces-around-statements #3699

Merged

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Feb 8, 2022

Description

This PR adds readability-braces-around-statements to .clang-tidy. It is based on experimental work under PR #3698.

A few manual fix-ups are needed in preparation, to work around clang-tidy bugs. The rest of the changes are automatically generated by running clang-tidy.

Alternative considered: readability-misleading-indentation

That would be more similar to Python and save us a bunch of lines with closing braces, but consider that C++ has a lot more syntactical noise and, unlike Python, comes with a preprocessor. For example:

     template <size_t... Is>
     bool load_impl_sequence(function_call &call, index_sequence<Is...>) {
 #ifdef __cpp_fold_expressions
-        if ((... || !std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is])))
+        if ((... || !std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is]))) {
             return false;
+        }
 #else
-        for (bool r : {std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is])...})
-            if (!r)
+        for (bool r : {std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is])...}) {
+            if (!r) {
                 return false;
+            }
+        }
 #endif
         return true;
     }

Without braces constructs like this are almost certain to cause confusion while debugging, or when going through with large-scale changes. The braces seem very useful as visual anchors, protecting against misreading the code.

See also: #3698 (comment)

Suggested changelog entry:

clang-tidy `readability-braces-around-statements` is now enforced pre-commit.

@rwgk rwgk changed the title V2 clang-tidy readability-braces-around-statements clang-tidy readability-braces-around-statements Feb 8, 2022
@rwgk rwgk requested review from Skylion007 and henryiii February 8, 2022 01:16
@rwgk rwgk marked this pull request as ready for review February 8, 2022 01:17
Ralf W. Grosse-Kunstleve added 2 commits February 8, 2022 12:26
clang-tidy automatic changes. NO manual changes.
@rwgk rwgk force-pushed the v2_clang-tidy_readability-braces-around-statements branch from d920d47 to cbdc21f Compare February 8, 2022 20:31
@rwgk
Copy link
Collaborator Author

rwgk commented Feb 8, 2022

The last force push did something unusual that's not reflected under the force-pushed link:

I squashed two commits into one, so that the addition to .clang-tidy goes with the automatic changes in one commit.

My plan is to merge this PR without squashing the remaining two commits, so that it is permanently clear and easy to see which changes were manual, and which automatic.

The net diff is exactly as before.

@rwgk rwgk merged commit ddbc74c into pybind:master Feb 8, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 8, 2022
@rwgk rwgk deleted the v2_clang-tidy_readability-braces-around-statements branch February 8, 2022 21:05
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants