Updated ruleBlockIfNoExit to better select branch if both are noexit. #19
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@refacto-visz |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the decompiler's handling of conditional control flow, specifically when both branches of an 'if' statement lead to a non-exiting state (e.g., both return or throw). By introducing new metrics like block depth and detailed PcodeOp analysis, the decompiler can now apply a more sophisticated heuristic to select the 'best' branch for collapsing, leading to more accurate and readable decompiled output for such scenarios. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
codeant-ai
bot
added
the
size:L
feat: Improve branch selection for if-else statements with dual returnsTL;DR: This change introduces a smarter heuristic for structuring Refacto PR SummaryThis pull request refactors the control-flow structuring rule To address this, a new heuristic has been implemented to "better" select the branch. This is achieved by introducing a concept of "block depth" to measure the complexity of control-flow nesting. The new logic prefers to collapse the branch with the shallower block depth into the Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant CS as CollapseStructure
participant Rule as ruleBlockIfNoExit
participant Select as selectBestNoExit
participant B0 as Branch 0
participant B1 as Branch 1
CS->>Rule: Analyze conditional block `bl`
Rule->>Rule: Verify `bl` has 2 "no exit" branches
Note over Rule: Both branches lead to a return or throw.
Rule->>Select: selectBestNoExit(branch_0, branch_1)
Select->>B0: getBlockDepth()
B0-->>Select: depth0
Select->>B1: getBlockDepth()
B1-->>Select: depth1
alt depth0 < depth1
Select-->>Rule: Return index 0
else depth1 < depth0
Select-->>Rule: Return index 1
else Depths are equal
Select->>Select: Apply tie-breaking rules (e.g., check for simple returns)
Select-->>Rule: Return best index
end
Rule->>CS: Restructure graph with chosen branch in `if` body
Testing GuideClick to expandTo verify this change, you can test its effect on a function where an
|
|
Warning Rate limit exceeded@visz11 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
CodeAnt AI finished reviewing your PR. |
Code Review: Decompiler Heuristic Refinements👍 Well Done
📁 Selected files for review (5)
🎯 Custom Instructions
📝 Additional Comments
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a more sophisticated heuristic for selecting a branch in if statements where both branches are no-exit. This is achieved by calculating and comparing the block depth of the branches. The changes include adding getBlockDepth() methods to various FlowBlock subclasses and a new getOpSize() method.
The overall logic is sound and improves the decompiler's output. My review includes a few suggestions to improve code readability, style consistency, and const-correctness. Notably, getOpSize() should be a const method, which is a crucial change for correctness and good design. I've also suggested using modern C++ features like range-based for loops and nullptr to make the code cleaner.
| list<PcodeOp *>::const_iterator beginOp(void) const { return op.begin(); } ///< Return an iterator to the beginning of the PcodeOps | ||
| list<PcodeOp *>::const_iterator endOp(void) const { return op.end(); } ///< Return an iterator to the end of the PcodeOps | ||
| bool emptyOp(void) const { return op.empty(); } ///< Return \b true if \b block contains no operations | ||
| int4 getOpSize(void); ///< Number of PcodeOps contained in \b this block |
There was a problem hiding this comment.
The getOpSize method does not modify the state of the BlockBasic object. It should be declared as const to enforce this and allow it to be called on const objects. This improves const-correctness and can prevent potential issues elsewhere.
int4 getOpSize(void) const; ///< Number of PcodeOps contained in \b this block| return hasOnlyMarkers(); | ||
| } | ||
|
|
||
| int4 BlockBasic::getOpSize(void) |
There was a problem hiding this comment.
The implementation of getOpSize should be marked as const to match the declaration in the header file. This method does not modify the object's state.
int4 BlockBasic::getOpSize(void) const| int4 BlockGraph::getInnerBlockDepth(void) | ||
|
|
||
| { | ||
| int4 depth; | ||
| int4 maxDepth = 0; | ||
| for(int4 i=0;i<list.size();++i){ | ||
| depth = list[i]->getBlockDepth(); | ||
| if(depth>maxDepth){ | ||
| maxDepth=depth; | ||
| } | ||
| } | ||
| return maxDepth; | ||
| } |
There was a problem hiding this comment.
The implementation of this loop can be made more concise and readable by using a range-based for loop. Also, the indentation for maxDepth=depth; seems to be off (using a tab instead of spaces), which affects code consistency. Declaring depth inside the loop scope is also better practice.
int4 BlockGraph::getInnerBlockDepth(void)
{
int4 maxDepth = 0;
for (const FlowBlock *block : list) {
const int4 depth = block->getBlockDepth();
if (depth > maxDepth) {
maxDepth = depth;
}
}
return maxDepth;
}| int4 BlockSwitch::getBlockDepth(void){ | ||
| int4 i; | ||
| int4 maxDepth=0; | ||
| for(i=0;i<caseblocks.size();++i){ | ||
| int4 depth=caseblocks[i].block->getBlockDepth(); | ||
| if(depth>maxDepth){ | ||
| maxDepth=depth; | ||
| } | ||
| } | ||
| return maxDepth+2; // +1 for switch block and +1 for case/default block | ||
| } |
There was a problem hiding this comment.
This function's implementation can be improved for readability and consistency.
- The opening brace
{is on the same line as the function signature, which is inconsistent with the surrounding code style. - A range-based for loop would be more modern and readable than a C-style for loop.
- The indentation for
maxDepth=depth;appears to be incorrect (using a tab). - The variable
iis declared at the top of the function but only used in the loop.
int4 BlockSwitch::getBlockDepth(void)
{
int4 maxDepth = 0;
for (const auto &caseblock : caseblocks) {
const int4 depth = caseblock.block->getBlockDepth();
if (depth > maxDepth) {
maxDepth = depth;
}
}
return maxDepth + 2; // +1 for switch block and +1 for case/default block
}| bool isRet0 = clause0->lastOp()!=(PcodeOp *)0 && clause0->lastOp()->isStandardReturn(); | ||
| bool isRet1 = clause1->lastOp()!=(PcodeOp *)0 && clause1->lastOp()->isStandardReturn(); |
There was a problem hiding this comment.
For better code clarity and to adhere to modern C++ standards, it's recommended to use nullptr instead of the C-style (PcodeOp *)0 for null pointer checks.
bool isRet0 = clause0->lastOp() != nullptr && clause0->lastOp()->isStandardReturn();
bool isRet1 = clause1->lastOp() != nullptr && clause1->lastOp()->isStandardReturn();| FlowBlock* fb; | ||
| if(clause0->getType()==FlowBlock::t_copy){ | ||
| fb = ((BlockCopy*)clause0)->subBlock(0); | ||
| if(fb->getType()==FlowBlock::t_basic && ((BlockBasic*)fb)->getOpSize()==1) return 0; | ||
| } | ||
| if(clause1->getType()==FlowBlock::t_copy){ | ||
| fb = ((BlockCopy*)clause1)->subBlock(0); | ||
| if(fb->getType()==FlowBlock::t_basic && ((BlockBasic*)fb)->getOpSize()==1) return 1; | ||
| } |
There was a problem hiding this comment.
This block of code can be improved for safety and clarity:
- Use
static_castinstead of C-style casts. This is safer and more explicit about the cast's intent. - The
fbvariable is declared once and reused. It's better practice to declare variables in the narrowest scope possible.
Additionally, BlockBasic::getOpSize() should be a const method, as it doesn't modify the object's state. I've added separate comments for this.
if(clause0->getType()==FlowBlock::t_copy){
FlowBlock* fb = static_cast<BlockCopy*>(clause0)->subBlock(0);
if(fb->getType()==FlowBlock::t_basic && static_cast<BlockBasic*>(fb)->getOpSize()==1) return 0;
}
if(clause1->getType()==FlowBlock::t_copy){
FlowBlock* fb = static_cast<BlockCopy*>(clause1)->subBlock(0);
if(fb->getType()==FlowBlock::t_basic && static_cast<BlockBasic*>(fb)->getOpSize()==1) return 1;
}
CodeAnt-AI Description
Select the best non-fallthrough branch when both sides exit
What Changed
Impact
✅ Fewer incorrect conditional collapses✅ More accurate early-return detection✅ Clearer nested control flow after decompilation💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.