Remove Redundant Goto Statements

[ShounakDas101]

Solves issue https://github.com/vgvassilev/clad/issues/526. We can only remove the 'goto' label when the parent of the parent of the return statement is null.

[parth-07]

@ShounakDas101 I suggest we wait on this pull request until we all agree that it's useful.

I am not convinced that this pull request is useful. It's solving a very specific case. I believe it will only be beneficial if we can remove gotos in other general cases as well. @ShounakDas101 Please let me know your thoughts on this.

[ShounakDas101]

@parth-07 In this pull request (PR), I have decided to remove 'goto statements' only if the 'return statement' is the direct child node of the 'Compound statement' that contains the entire function body(This is the only case where we should remove 'goto statement'). The other 'return statements' will be within some control flow block. I believe that 'goto statements' for such return statements are essential and should not be removed.

Could you please suggest a test case where this approach fails? This PR has removed all redundant goto statements in the test cases available in CLAD.

[parth-07]

Hi @ShounakDas101

Could you please suggest a test case where this approach fails?

I did not say that your approach fails in any case. I just do not see any advantage of adding extra complexity to remove goto's for a special case of the return statement. For this to be beneficial, we also need to remove other goto's.

[ShounakDas101]

Hi @parth-07 I have tried to solve issue #526. I believe the issue specifically only wants to remove redundant goto statements like this ... goto _label0; _label0: ...

For this to be beneficial, we also need to remove other goto's.

By other goto's do you mean all goto's should be removed?

[parth-07]

I believe the issue specifically only wants to remove redundant goto statements like this

Yes, the issue specifically wants to remove gotos from the special case only. But I am still not sure if we benefit from this functionality. The compiler will optimize the redundant goto statements anyways.

The main concern is that goto statement prohibits further differentiation. Solving a special case wouldn't lift this restriction.

@vgvassilev What do you think about removing the redundant goto statements?

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[codecov[bot]]

Codecov Report

Merging #608 (6cf27f3) into master (790d978) will increase coverage by 0.01%. The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #608      +/-   ##
==========================================
+ Coverage   93.83%   93.84%   +0.01%     
==========================================
  Files          41       41              
  Lines        6032     6042      +10     
==========================================
+ Hits         5660     5670      +10     
  Misses        372      372              

Files Changed	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	98.70% <ø> (ø)
lib/Differentiator/ReverseModeVisitor.cpp	95.62% <100.00%> (+0.02%)	:arrow_up:

Files Changed	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	98.70% <ø> (ø)
lib/Differentiator/ReverseModeVisitor.cpp	95.62% <100.00%> (+0.02%)	:arrow_up:

[vgvassilev]

Fixed in #938