UPF: add upf_version#9743
Conversation
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.
src/upf/src/upf.i
Outdated
| 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);
}
src/upf/src/upf.tcl
Outdated
| 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.
3b647ad to
8eab67f
Compare
Signed-off-by: Divyanshu_Sharma <divyanshu88999@gmail.com>
8eab67f to
3aac0e7
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Title: UPF: add upf_version command support
This PR resolves part of Issue #5617 by implementing the
upf_versioncommandChanges made:
upf_versionAPI in upf.hBehavior:
upf_version <version>Implementation strategy:
This follows the incremental approach discussed in Issue #5617:
This PR corresponds to Phase 1:
Basic UPF command support
Next planned commands:
No documentation files were modified in this PR as per incremental workflow.
Related issue:
#5617