Skip to content

Algorithm Common: add braces to single statements#4567

Merged
ktf merged 1 commit into
AliceO2Group:devfrom
anerokhi:readability-braces-around-statements-2
Oct 17, 2020
Merged

Algorithm Common: add braces to single statements#4567
ktf merged 1 commit into
AliceO2Group:devfrom
anerokhi:readability-braces-around-statements-2

Conversation

@anerokhi
Copy link
Copy Markdown
Contributor

No description provided.

@ktf
Copy link
Copy Markdown
Member

ktf commented Oct 10, 2020

I think this kind of cosmetic commits do more harm than good. The chance to introduce issues while merging code with other developments is basically 1, for no real benefit (AFAICT 0 bugs in the last 5 years were due to missing braces). IMHO this should be implemented as part of the PR checks, proposing fixes only for the modified files, like the clang-format ones. What is preventing this from working like that?

@anerokhi anerokhi marked this pull request as draft October 10, 2020 07:03
@davidrohr
Copy link
Copy Markdown
Collaborator

+1 for adding this to the PR checks, although perhaps not so easy

Well, actually, I think I could do it with uncrustify, could work similarly to clang-format. It might be possible to disable all formatting requirements and just enforce the braces. But would we want a second formatter to check the code?

Personally, I don't have a real problem with merging this. If I forgot the braces and someone else fixes them, then I'll have to live with the merge collisions... :)

@anerokhi
Copy link
Copy Markdown
Contributor Author

Well, actually, I think I could do it with uncrustify, could work similarly to clang-format.

Similarly to clang-format-diff, there is clang-tidy-diff.

@anerokhi anerokhi marked this pull request as ready for review October 14, 2020 11:51
@ktf ktf merged commit b7bb4e9 into AliceO2Group:dev Oct 17, 2020
tklemenz pushed a commit to tklemenz/AliceO2 that referenced this pull request Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants