[clang-tidy] Vote: Control flow checks

[j-stephan]

This PR adds control flow checks to our .clang-tidy file.

Note: I intentionally left out the modernize-loop-convert check (Link) because it has compatibility issues with OpenMP.

Voting will close on 02 July 2021, 18:00 HZDR time.

• Do not repeat branches - Link
• Find obvious infinite loops - Link
• Find redundant branch conditions - Link
• Find suspicious semicolons - Link
• Find terminating continue - Link
• Detect too small loop variables - Link. Note: Currently this checks for 32-bit integers that are too small. If you want this changed (to 16 bit, for example), please note this in the comment field.
• Find temporaries that look like RAII objects - Link
• Do not use floating point types as loop counters - Link
• Avoid goto - Link
• Find missing code paths - Link. Note: In the current version the check will warn on missing else branches if there are if-else if branches before. If you want this turned off, please note this in the comment field.
• No runtime recursion - Link
• Put braces around statements - Link. Note: Currently the check allows for short statements (== 1 line) to exist without braces. If you want this changed, please note this in the comment field.
• Do not check for nullptr when calling delete - Link
• Do not use else after return - Link
• Find misleading indentation - Link
• Find redundant control flow statements - Link

Vote template

Check | Yes | No | Comment
------|-----|----|--------
Don't repeat branches | X | X | X
Find infinite loops | X | X | X
Find redundant branch conditions | X | X | X
Find suspicious semicolons | X | X | X
Find terminating `continue` | X | X | X
Detect too small loop variables | X | X | X
Find temporaries that look like RAII objects | X | X | X
No floating point loop counters | X | X | X
Avoid `goto` | X | X | X
Find missing code paths | X | X | X
No runtime recursion | X | X | X
Put braces around statements | X | X | X
No `nullptr` check when `delete`ing | X | X | X
No `else` after `return` | X | X | X
Find misleading indentation | X | X | X
Find redundant control flow statements | X | X | X

Vote

Check	Yes	No	Comment
Don't repeat branches	1	0	-
Find infinite loops	1	0	-
Find redundant branch conditions	1	0	-
Find suspicious semicolons	1	0	-
Find terminating continue	1	0	-
Detect too small loop variables	1	0	-
Find temporaries that look like RAII objects	1	0	-
No floating point loop counters	1	0	-
Avoid goto	1	0	-
Find missing code paths	1	0	-
No runtime recursion	1	0	-
Put braces around statements	1	0	-
No nullptr check when deleteing	1	0	-
No else after return	1	0	-
Find misleading indentation	1	0	-
Find redundant control flow statements	1	0	-

[bernhardmgruber]

Check	Yes	No	Comment
Don't repeat branches	2	0	-
Find infinite loops	2	0	-
Find redundant branch conditions	2	0	-
Find suspicious semicolons	2	0	-
Find terminating continue	2	0	-
Detect too small loop variables	2	0	-
Find temporaries that look like RAII objects	2	0	-
No floating point loop counters	2	0	In principle you can use float as induction variable, but you have to know what you are doing. So I am fine with clang-tidy flagging them and putting a NOLINT comment in case I need it.
Avoid goto	2	0	~goto is useful in specific scenarios and most people should know to use it sparingly.~
Find missing code paths	2	0	That one might be too aggressive, but let's try it and find out!
No runtime recursion	1	1	There is nothing wrong with recursion :) Or I missunderstood the check.
Put braces around statements	1	1	Unnecessarily increases vertical space.
No nullptr check when deleteing	2	0	-
No else after return	2	0	-
Find misleading indentation	2	0	-
Find redundant control flow statements	2	0	-

[j-stephan]

goto is useful in specific scenarios and most people should know to use it sparingly.

The only scenario I can think of is explicitly mentioned in the check description (and not flagged by clang-tidy).

[bernhardmgruber]

goto is useful in specific scenarios and most people should know to use it sparingly.

The only scenario I can think of is explicitly mentioned in the check description (and not flagged by clang-tidy).

I just read the first line of the check documentation. Thanks for pointing this out! I changed my vote.

[SimeonEhrig]

Check	Yes	No	Comment
Don't repeat branches	3	0	-
Find infinite loops	3	0	-
Find redundant branch conditions	3	0	-
Find suspicious semicolons	3	0	-
Find terminating continue	3	0	-
Detect too small loop variables	2	0	-
Find temporaries that look like RAII objects	3	0	-
No floating point loop counters	2	1	I'm not sure, if this is necessary. If I use floating point numbers, I pretty sure, what I do. I would never use it accidentally.
Avoid goto	3	0	-
Find missing code paths	3	0	let's simply try it
No runtime recursion	1	1	-
Put braces around statements	1	1	I can live with both
No nullptr check when deleteing	3	0	-
No else after return	3	0	-
Find misleading indentation	3	0	-
Find redundant control flow statements	3	0	-

[j-stephan]

If I use floating point numbers, I pretty sure, what I do. I would never use it accidentally.

Well, we're not just doing this for ourselves, but also for future colleagues who may not be aware of floating point comparison issues ;-).

There is nothing wrong with recursion :) Or I missunderstood the check.

Too much recursion can lead to a stack overflow. That is probably the reason for the check's existence.

Unnecessarily increases vertical space.

In the current version, this is still allowed:

if(something)
    ++counter;

It will only reformat statements with at least two lines:

if(something)
    my_counter_inc(counter,
                   another_param,
                   and_another_param);

[bernhardmgruber]

If I use floating point numbers, I pretty sure, what I do. I would never use it accidentally.

Well, we're not just doing this for ourselves, but also for future colleagues who may not be aware of floating point comparison issues ;-).

Sure. That's why I voted for the check. And yes, there are a lot of myths about float comparisons, and the topic needs a deeper understanding.

There is nothing wrong with recursion :) Or I missunderstood the check.

Too much recursion can lead to a stack overflow. That is probably the reason for the check's existence.

I know. But that is not a good enough reason to just forbid it. For some algorithms recursion is just way easier and sometimes more efficient to implement. There is nothing wrong with e.g. quicksort, or walking a tree, or merge sort.

Unnecessarily increases vertical space.

In the current version, this is still allowed:

if(something)
    ++counter;

It will only reformat statements with at least two lines:

if(something)
    my_counter_inc(counter,
                   another_param,
                   and_another_param);

I have seen that. But also the second version is totally fine for me!

[psychocoderHPC]

Check	Yes	No	Comment
Don't repeat branches	4	0	-
Find infinite loops	4	0	If we need a infinite loop we can disable clang-tidy for the required code
Find redundant branch conditions	4	0	I wrote some code snipped in PIConGPU where the redundancy is required to avoid warnings because loops indices were compile-time values. Sometimes, very seldom, redundant branches are required.
Find suspicious semicolons	4	0	-
Find terminating continue	4	0	-
Detect too small loop variables	3	0	-
Find temporaries that look like RAII objects	4	0	-
No floating point loop counters	2	2	If I need to run an optimizer I need a floating point comparison as loop condition
Avoid goto	4	0	-
Find missing code paths	3	1	lf I have a cascade of if conditions to bring my code in a well defined state I do not like to have else branches.
No runtime recursion	1	2	I do not understand why the concept of runtime recursion should be avoided. A simple quicksort shows the need for recursion.
Put braces around statements	1	2	-
No nullptr check when deleteing	3	1	IMO not required: https://stackoverflow.com/a/6731484
No else after return	4	0	-
Find misleading indentation	4	0	-
Find redundant control flow statements	4	0	-

[sbastrakov]

For float as loop variable. It is obviously not the most standard usage. But i see no reason to forbid it

[j-stephan]

@psychocoderHPC Ready to merge.