grt: fix is_incremental_ flag not reset in endIncremental()#10039
grt: fix is_incremental_ flag not reset in endIncremental()#10039Divinesoumyadip wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
endIncremental() was setting is_incremental_ = true instead of false, causing the router to remain in incremental mode permanently after the incremental routing session ends. Fixes: GlobalRouter.cpp line 336 Signed-off-by: Divinesoumyadip <soumyacode7@gmail.com>
![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 fixes a bug in GlobalRouter::endIncremental where the is_incremental_ flag was incorrectly set to true instead of false. The review feedback suggests moving this flag update to the end of the function to ensure exception safety and maintain state consistency in case subsequent operations fail.
| is_incremental_ = false; | ||
| fastroute_->setResistanceAware(resistance_aware_); | ||
| updateDirtyRoutes(save_guides); | ||
| grouter_cbk_->removeOwner(); |
There was a problem hiding this comment.
For improved exception safety and to ensure transactional correctness, it's better to modify the state flag is_incremental_ only after all operations that could potentially fail have successfully completed. If updateDirtyRoutes() were to throw an exception, the system would be left in an inconsistent state where is_incremental_ is false, but the incremental session hasn't been properly terminated.
By moving the flag update to after the main work is done, we ensure that the state remains consistent even in the case of an error during the termination process.
fastroute_->setResistanceAware(resistance_aware_);
updateDirtyRoutes(save_guides);
grouter_cbk_->removeOwner();
is_incremental_ = false;
|
clang-tidy review says "All clean, LGTM! 👍" |
| void GlobalRouter::endIncremental(bool save_guides) | ||
| { | ||
| is_incremental_ = true; | ||
| is_incremental_ = false; |
There was a problem hiding this comment.
Actually, it is intended that at the beginning of endIncremental this flag is true, since incremental updates are made below with the updateDirtyRoutes call. The proper fix is to set is_incremental_ to false at the end of the function
2 participants
endIncremental()was settingis_incremental_ = trueinstead of false. This means the router never actually exits incremental mode after the session ends, which can cause incorrect behavior in subsequent routing calls that check this flag.is_incremental_ = falseinendIncremental(), which is the correct behavior since the function's purpose is to terminate theincremental routing session.