Remove Redundant Goto Statements #608
Conversation
|
@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 |
|
@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. |
I did not say that your approach fails in any case. I just do not see any advantage of adding extra complexity to |
Yes, the issue specifically wants to remove The main concern is that @vgvassilev What do you think about removing the redundant |
| }else{ | ||
| OnlyReturn=true; | ||
| } | ||
| } |
There was a problem hiding this comment.
We should be able to detect in a smarter way if we need to emit a goto-statement given a forward sweep and then conditionally emit it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -744,6 +744,20 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, | |||
| beginBlock(direction::forward); | |||
| beginBlock(direction::reverse); | |||
| for (Stmt* S : CS->body()) { | |||
| std::string cppString="ReturnStmt"; | |||
| const char* cString = S->getStmtClassName(); | |||
| if(cppString.compare(cString) == 0 ){ | |||
There was a problem hiding this comment.
warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
| if(cppString.compare(cString) == 0 ){ | |
| if(cppString == cString ){ |
| if (!parents.empty()){ | ||
| const Stmt* parentStmt = parents[0].get<Stmt>(); | ||
| if(parentStmt==nullptr) | ||
| OnlyReturn=true; |
There was a problem hiding this comment.
warning: redundant boolean literal in conditional assignment [readability-simplify-boolean-expr]
lib/Differentiator/ReverseModeVisitor.cpp:752:
- if(parentStmt==nullptr)
- OnlyReturn=true;
- else
- OnlyReturn=false;
+ OnlyReturn = parentStmt==nullptr;
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report
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
|
|
Fixed in #938 |
Solves issue #526. We can only remove the 'goto' label when the parent of the parent of the return statement is null.