Skip to content

STYLE: Use braces for single line conditional loop body statements#3868

Closed
jhlegarreta wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
jhlegarreta:UseBracesForSingleLineConditionalLoops
Closed

STYLE: Use braces for single line conditional loop body statements#3868
jhlegarreta wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
jhlegarreta:UseBracesForSingleLineConditionalLoops

Conversation

@jhlegarreta
Copy link
Copy Markdown
Member

Use braces also for single line conditional loop body statements following the ITK SW style guidelines.

PR Checklist

Use braces also for single line conditional loop body statements
following the ITK SW style guidelines.
@github-actions github-actions Bot added area:Core Issues affecting the Core module area:Python wrapping Python bindings for a class type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Jan 14, 2023
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK of course, thanks Jon, but doesn't this occur on many more places? For example

if (signedVariable < 0)
return false;
if (unsignedVariable > static_cast<size_t>(itk::NumericTraits<TSignedInt>::max()))
return false;

You may find them all by a regular expression like if \(.+\)\r\n [ ]+[^ {] According to Notepad++ (on Windows), it has 265 hits in 109 files (using Filter = itk*.h;itk*.hxx;itk*.cxx)

@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Jan 14, 2023

@N-Dekker Of course, it does. It was not meant to be a comprehensive PR at all; I just came across this while looking at other things, and I did not use a regex. However, I would very much appreciate a sweeping PR, but please contribute a script to the Utilities folder as well.

Edit: Include Examples, non-standard modules, and for and while loops.

@N-Dekker
Copy link
Copy Markdown
Contributor

Thanks for explaining why you only addressed this particular case, Jon. I don't have time to address the other 200+ cases right now. I want to focus now on getting the initialization done (including PR #3851). If it is a priority to you, the regular expression that I posted ( if \(.+\)\r\n [ ]+[^ {]) may be of help to you. Just my two cent.

@jhlegarreta
Copy link
Copy Markdown
Member Author

If it is a priority to you, the regular expression that I posted ( if (.+)\r\n [ ]+[^ {]) may be of help to you. Just my two cent.

The regex is of great help to find them, and I'd very much like to have them changed, ideally with the regex and a script, but am also focusing on other changes now.

@jhlegarreta
Copy link
Copy Markdown
Member Author

Alternatively, we can set the state of this PR to draft or open an issue. What do you think @N-Dekker ?

@jhlegarreta jhlegarreta marked this pull request as draft January 14, 2023 23:08
@N-Dekker
Copy link
Copy Markdown
Contributor

Alternatively, we can set the state of this PR to draft or open an issue. What do you think @N-Dekker ?

Looks like a good plan. By the way, I would just do the find-replace in the editor (Notepad++) or IDE (Visual Studio), so then I would not really have a script file. The main thing is to get the regular expression right. And it seems sufficient to me to just mention the regular expression in the commit message and/or the pull request. But of course, if you could write a script file (bash/bat/shell/python or even C++), feel free to do so.

Note that the style improvement to add braces to single statements can be done with a find-replace in an editor, while for the in-class {} member initializers (PR #3851) I really needed some kind of scripting, because I did not want to "blindly" add those {} braces everywhere. The script was there to specifically look for classes that have an itkNewMacro macro call.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Jan 20, 2023

Superseded by #3875.

@dzenanz dzenanz closed this Jan 20, 2023
@jhlegarreta jhlegarreta deleted the UseBracesForSingleLineConditionalLoops branch January 20, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Python wrapping Python bindings for a class type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants