odb: impl 3dblox checker checkConnectionRegions#9515
Conversation
Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.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 implements a new checker, checkConnectionRegions, in 3dblox to validate all regions in all connections. The checker verifies that connections are well-defined, checking for missing regions in the unfolded model and validating geometric properties like intersection, orientation, and thickness for existing regions. The changes are well-tested with new unit tests covering various invalid connection scenarios and a new integration test for missing regions. The implementation is clear, and the suggestion to improve a log message for better accuracy is valid.
src/odb/src/3dblox/checker.cpp
Outdated
| } | ||
| if (count > 0) { | ||
| logger_->warn( | ||
| utl::ODB, 273, "Found {} non-intersecting connections", count); |
There was a problem hiding this comment.
The warning message "Found {} non-intersecting connections" is not entirely accurate. The isValid function checks for more than just intersections, including incorrect facing sides of regions and thickness mismatches. A more general message like "Found {} invalid connections" would be more appropriate and less misleading for the user.
| utl::ODB, 273, "Found {} non-intersecting connections", count); | |
| utl::ODB, 273, "Found {} invalid connections", count); |
|
clang-tidy review says "All clean, LGTM! 👍" |
src/odb/src/3dblox/checker.cpp
Outdated
| if ((!conn.top_region || !conn.bottom_region) | ||
| && conn.connection->getTopRegion() | ||
| && conn.connection->getBottomRegion()) { |
There was a problem hiding this comment.
I added it because of the parser error. I think we should fix the error in a separate PR. But, should we keep the check as defensive programming? Or is it confusing?
There was a problem hiding this comment.
No I think it is confusing.
There was a problem hiding this comment.
OK will remove it from this PR now.
src/odb/src/3dblox/checker.cpp
Outdated
| } | ||
| if (count > 0) { | ||
| logger_->warn( | ||
| utl::ODB, 273, "Found {} non-intersecting connections", count); |
| HierChiplet: | ||
| type: hier | ||
| regions: | ||
| hier_reg: | ||
| side: front |
There was a problem hiding this comment.
This is a good example of a parser error that should be marked earlier as hierarchical chiplets should be purely hierarchical and never have regions defined.
Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
src/odb/src/3dblox/checker.cpp
Outdated
| void Checker::checkConnectionRegions(dbMarkerCategory* top_cat, | ||
| const UnfoldedModel& model) | ||
| { | ||
| auto* cat = dbMarkerCategory::createOrReplace(top_cat, "Connection regions"); |
There was a problem hiding this comment.
Create only if there is a marker
Do not create the marker unless there are violations Signed-off-by: Ahmed R. Mohamed <ahmed@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
Check all regions in all connections are well defined.
Related to: #9291