Revert "Undo the pad changes and save them for later"#10314
Conversation
![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 improves the stability and determinism of the RDL router by incorporating lexicographical comparisons (names and IDs) into sorting logic and map keys. It also introduces a tie-breaking direction bias in edge weight calculations. Review feedback focuses on performance optimizations, specifically recommending the use of std::string_view instead of std::string to avoid unnecessary heap allocations during frequent comparisons. Additionally, it is suggested to replace a hardcoded magic number in the edge weight logic with a named constant and provide a clarifying comment regarding the heuristic rationale.
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
gadfort
left a comment
gadfort
left a comment
There was a problem hiding this comment.
I'm not sure why the string compares are needed (seems very expensive vs comparing the DB IDs).
Also in all cases if the names match the id's will also match correct?
This reverts commit d7e11c6. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
dbITerm::getName() returns std::string by value (constructed on the fly), so binding the result to std::string_view leaves the view aliasing a destroyed temporary at the next statement. Switch the two sort/compare sites in RDLRoute to const std::string& -- lifetime extension of the temporary keeps the reference valid -- and apply the same const-ref form to the DbNetPtrLess / DbITermPtrLess comparators in RDLRouter.h to avoid an unnecessary copy. Drop the now-unused <map> include from RDLRoute.h and the <string_view>/<tuple> includes from RDLRoute.cpp. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
|
@gadfort I've rebased, please review. |
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
gadfort
left a comment
gadfort
left a comment
There was a problem hiding this comment.
Why are two of the tests listed as pass/fail?
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
The both check for an expected failure. |
|
@maliberty but why the need to change them for bazel? We have other tests that check to failures. |
The change is for both cmake & bazel. They don't have to be changed but they are essentially pass/fail tests so I assume the AI switched them to match others. If you prefer I can change them back. |
|
@maliberty those changes are not needed, it seems like reverting them back would be nice. |
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
3 participants
This reverts commit d7e11c6.
Summary
Make rdl more deterministic
Type of Change
Impact
Makes the tests pass in bazel and cmake
Verification
./etc/Build.sh).Related Issues
[Link issues here]