odb: accept numeric edge type names in macro LEF58_EDGETYPE parser#10965
Conversation
The LEF58_EDGETYPE macro property parser used the shared _string rule, which requires the edge type name to start with an alphabetic character. Numeric names such as "2" failed to parse and were rejected with ODB-0299. Use a rule that accepts any non-blank token for the edge type name, matching the earlier CELLEDGESPACINGTABLE fix for issue The-OpenROAD-Project#10668. Fixes The-OpenROAD-Project#10953 Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
|
@codex review |
![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 updates the LEF macro edge type parser to support numeric edge type names (such as "2") by introducing a new EDGE_TYPE parser rule, and adds corresponding regression tests. A review comment suggests improving the robustness of the parser rule by subtracting space instead of blank - '\n' to correctly handle carriage return (\r) characters in files with Windows/DOS line endings.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Matt Liberty <matt.liberty@gmail.com>
| // Edge type names may start with any non-blank character (e.g. "2") | ||
| qi::rule<std::string::const_iterator, std::string(), space_type> EDGE_TYPE | ||
| = lexeme[+(char_ - space - ';')]; |
There was a problem hiding this comment.
I remember specifically that I defined the _string rule to start with an alphabet in order to disallow parsing a number as a string. If the more permissive rule is actually the more correct one, maybe we should just generalize it into _string
There was a problem hiding this comment.
The LEF manual gives no clear guidance on what is legal. I did experiment with the SITE <> construct and <> can be 1 or even 1.1. The LEF parser specifically disables numeric parsing where strings are expected. The suggests this should be allowed.
There was a problem hiding this comment.
Likely there are more places to update but I left that for future work.
4 participants
Fixes #10953
The LEF58_EDGETYPE macro property parser used the shared
_stringrule, which requires the edge type name to start with an alphabetic character. Numeric names such as2failed to parse and were rejected with ODB-0299, dropping the edge type from the database.This replaces
_stringwith a local rule that accepts any non-blank token for the edge type name, mirroring the earlier fix for the same problem in theCELLEDGESPACINGTABLEparser (#10668). The shared_stringrule is left untouched since other parsers rely on its alpha-first behavior for disambiguation.Adds a regression test (
Fixture.TestLef58MacroNumericEdgeType) covering numeric names as well as the pre-existing alpha names andRANGEclause; the test fails without the parser change.