Skip to content

odb: add versions to make database versions consistent#9710

Closed
gadfort wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
gadfort:db-verison
Closed

odb: add versions to make database versions consistent#9710
gadfort wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
gadfort:db-verison

Conversation

@gadfort
Copy link
Copy Markdown
Collaborator

@gadfort gadfort commented Mar 10, 2026

No-op but needed to allow version 127 databases to be parsed

Signed-off-by: Peter Gadfort <peter.gadfort@gmail.com>
@gadfort gadfort requested a review from maliberty March 10, 2026 20:02
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the database schema version and adds new constants to support parsing older database versions. My review focuses on improving the naming of the new constants for better consistency and clarity within the codebase. I've suggested renaming the new schema constants to include a DbGuide prefix, aligning them with existing naming conventions for dbGuide-related changes.

Comment on lines +56 to +59
inline constexpr uint32_t kSchemaIgnoreUsageRemoved = 128;

// Revision where the ignore_usage_ flag was added to dbGuide
inline constexpr uint32_t kSchemaIgnoreUsage = 127;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other schema version constants related to dbGuide (e.g., kSchemaDbGuideCongested), it would be clearer to prefix these new constants with DbGuide. This improves readability and makes it easier to find all changes related to a specific database object.

Suggested change
inline constexpr uint32_t kSchemaIgnoreUsageRemoved = 128;
// Revision where the ignore_usage_ flag was added to dbGuide
inline constexpr uint32_t kSchemaIgnoreUsage = 127;
inline constexpr uint32_t kSchemaDbGuideIgnoreUsageRemoved = 128;
// Revision where the ignore_usage_ flag was added to dbGuide
inline constexpr uint32_t kSchemaDbGuideIgnoreUsage = 127;

@maliberty maliberty enabled auto-merge March 10, 2026 20:14
@maliberty
Copy link
Copy Markdown
Member

@eder-matheus FYI

inline constexpr uint32_t kSchemaIgnoreUsageRemoved = 128;

// Revision where the ignore_usage_ flag was added to dbGuide
inline constexpr uint32_t kSchemaIgnoreUsage = 127;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kSchemaIgnoreUsage wasn't intended to be in master branch. I thought it was removed from my grt guides restore PR before the merge.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eder-matheus The issue here was that in your PR you dropped the DB version from 127 to 126. Once the version number if on master, it cannot be changed down otherwise it breaks any DBs created with it.

@gadfort
Copy link
Copy Markdown
Collaborator Author

gadfort commented Mar 11, 2026

This is moot with: #9697
Since 127 is now used, again, any DB with version 127 will fail to parse if it was created from the previous version.

@gadfort gadfort closed this Mar 11, 2026
auto-merge was automatically disabled March 11, 2026 12:08

Pull request was closed

@gadfort gadfort deleted the db-verison branch March 11, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants