Improve NoreturnContractCheck#441
Conversation
This is to facilitate the distinction between routine exits and exceptional exits from a control flow graph.
This is being removed in favour of the type system and a more generic approach to block identification. Additionally, there may be more than one exit block in a CFG.
This fixes a bug that allowed loop control flow statements to be used inside `try finally` statements with the existance of a loop.
| import au.com.integradev.delphi.cfg.api.RoutineExit; | ||
| import au.com.integradev.delphi.cfg.api.UnconditionalJump; | ||
| import au.com.integradev.delphi.cfg.api.UnknownException; | ||
| import au.com.integradev.delphi.cfg.api.*; |
There was a problem hiding this comment.
Let's use explicit imports (you may need an IDE configuration change.)
| import au.com.integradev.delphi.cfg.api.RoutineExit; | ||
| import au.com.integradev.delphi.cfg.api.Terminated; | ||
| import au.com.integradev.delphi.cfg.api.UnknownException; | ||
| import au.com.integradev.delphi.cfg.api.*; |
There was a problem hiding this comment.
Let's use explicit imports.
| import java.util.HashSet; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
| import java.util.*; |
There was a problem hiding this comment.
Let's use explicit imports.
|
|
||
| // Remove the exceptional exit if there are no paths to it | ||
| Block exceptionalExitBlock = | ||
| ListUtils.reverse(blocks).stream() |
There was a problem hiding this comment.
Can we use the guava Lists.reverse instead? I don't like the idea of using these super specific collection utils from sonarsource analyzer commons (and trust the long-term stability of Guava's API much more.)
| import au.com.integradev.delphi.cfg.api.ControlFlowGraph; | ||
| import au.com.integradev.delphi.cfg.api.Terminated; | ||
| import au.com.integradev.delphi.cfg.api.UnconditionalJump; | ||
| import au.com.integradev.delphi.cfg.api.*; |
There was a problem hiding this comment.
Let's use explicit imports.
There was a problem hiding this comment.
The previous implementation passes all tests if we simply change the implementation of notFinallyOrExit:
private static boolean notFinallyOrExit(Block block) {
return !(block instanceof Finally || block instanceof ExceptionalRoutineExit);
}What's the benefit of the full-on implementation rejig?
If there's some case that this handles that the previous implementation fundamentally doesn't, can we add unit tests to demonstrate that?
There was a problem hiding this comment.
The rejig made more sense to me. After playing with the existing implementation a bit more, I think the change you suggested is sufficient. I will revert it to that.
jgardn3r
force-pushed
the
no-return-except-block
branch
from
33df283 to
da15b24
Compare
2 participants
RoutineExittype to control flow graph typesControlFlowGraph::getExitBlockContinueandBreakaroundtry finallyRoutineExitblock is always id 0withFailMessagewithasin CFGBlockCheckerExceptionalRoutineExitto appropriateControlFlowGraphsNoreturnContractCheck