UPF: add upf_version#9743
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 introduces the upf_version command. The implementation is straightforward, but there's an issue with error handling. Currently, if an invalid version string is provided, the Tcl command succeeds while only printing a warning. My feedback focuses on propagating the error status from the C++ layer to the Tcl script to ensure the command fails correctly, which is more consistent with typical command-line tool behavior.
| void upf_version_cmd(const char* version) { | ||
| odb::dbDatabase* db = getOpenRoad()->getDb(); | ||
| upf::upf_version(getOpenRoad()->getLogger(), db->getChip()->getBlock(), version); | ||
| } |
There was a problem hiding this comment.
The upf_version_cmd function should propagate the success/failure status from upf::upf_version. This allows the Tcl script to handle errors properly. Please change the return type to bool and return the result of the call.
bool upf_version_cmd(const char* version) {
odb::dbDatabase* db = getOpenRoad()->getDb();
return upf::upf_version(getOpenRoad()->getLogger(), db->getChip()->getBlock(), version);
}
| sta::check_argc_eq1 "upf_version" $args | ||
|
|
||
| set version [lindex $args 0] | ||
| upf::upf_version_cmd $version |
There was a problem hiding this comment.
The Tcl command should check the return value of upf::upf_version_cmd and report an error if it fails. This ensures that invalid calls (e.g., with an empty version string) cause the Tcl command to fail, which is standard practice. This change depends on the suggested change in src/upf/src/upf.i.
if {![upf::upf_version_cmd $version]} {
sta::error "Failed to set UPF version."
}
There was a problem hiding this comment.
Bro I Updated the implementation to propagate the return status
from C++ to Tcl as per your suggestion
Changed upf_version_cmd to return bool, Updated Tcl to check return value, Fixed function signature in header
Also fixed my commit with Signed-off-by for DCO.
Please review again.
DivyanshuX9
force-pushed
the
Upf_5617_issue_resolve_by_Divyanshu
branch
3 times, most recently
from
3b647ad to
8eab67f
Compare
Signed-off-by: Divyanshu_Sharma <divyanshu88999@gmail.com>
DivyanshuX9
force-pushed
the
Upf_5617_issue_resolve_by_Divyanshu
branch
from
8eab67f to
3aac0e7
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
| logger->warn(utl::UPF, 62, "upf_version requires a version string"); | ||
| return false; | ||
| } | ||
| logger->info(utl::UPF, 1, "UPF version set to {}", version); |
There was a problem hiding this comment.
This doesn't do anything besides print a message.
False advertising |
- Add setUPFVersion/getUPFVersion to dbBlock (odb layer) - Bump schema to kSchemaBlockUpfVersion=133 for DB stream compat - Implement set_upf_version in upf.cpp; validates and persists version - Fix duplicate logger ID: use UPF 46 (was colliding with UPF 1) - Add upf_version Tcl command with proper failure propagation - Add SWIG wrapper set_upf_version_cmd in upf.i - Write version to UPF output file via writer.cpp - Add upf_version regression test (valid, multi-call, invalid-empty) Fixes The-OpenROAD-Project#5617 Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>
2 participants
upf: validate and persist upf_version
This adds support for parsing upf_version, validates supported UPF versions
(1.0, 2.0, 2.1, 3.0, 3.1), stores the selected version on dbBlock, and writes
it back out through write_upf.
Adds regression coverage for:
This is an incremental step toward broader UPF support.