[Relax] Enhance unique block name generation with numeric suffixes#18554
[Relax] Enhance unique block name generation with numeric suffixes#18554tlopex merged 2 commits intoapache:mainfrom
Conversation
Summary of ChangesHello @guan404ming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the block name deduplication process within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 enhances the unique block name generation in fuse_tir.cc to correctly handle numeric suffixes, resolving a TODO item. The implementation is sound and is accompanied by a new test case that verifies the improved behavior. My review includes a suggestion to further improve the robustness and readability of the C++ code by handling potential integer overflows and exceptions, and by clarifying the logic for generating new names.
| ffi::String GetUniqueName(const ffi::String& prefix) { | ||
| ffi::String unique_prefix = prefix; | ||
| auto it = name_count_.find(prefix); | ||
| while (name_count_.count(unique_prefix)) { | ||
| unique_prefix = prefix + "_" + std::to_string(++it->second); | ||
| std::string str_prefix = std::string(prefix); | ||
|
|
||
| // Find where the trailing digits start | ||
| size_t base_len = str_prefix.length(); | ||
| while (base_len > 0 && std::isdigit(str_prefix[base_len - 1])) { | ||
| --base_len; | ||
| } | ||
|
|
||
| std::string base_name; | ||
| int start_num = 0; | ||
|
|
||
| if (base_len < str_prefix.length()) { | ||
| base_name = str_prefix.substr(0, base_len); | ||
| start_num = std::stoi(str_prefix.substr(base_len)); | ||
| } else { | ||
| base_name = str_prefix; | ||
| } | ||
|
|
||
| // Check if the original name is available | ||
| ffi::String candidate = prefix; | ||
| if (!name_count_.count(candidate)) { | ||
| name_count_[candidate] = 0; | ||
| return candidate; | ||
| } | ||
|
|
||
| // Generate unique name by incrementing the numeric suffix | ||
| int counter = (start_num > 0) ? start_num + 1 : 1; | ||
| while (true) { | ||
| candidate = ffi::String(base_name + std::to_string(counter)); | ||
| if (!name_count_.count(candidate)) { | ||
| name_count_[candidate] = 0; | ||
| return candidate; | ||
| } | ||
| ++counter; | ||
| } | ||
| name_count_[unique_prefix] = 0; | ||
| return unique_prefix; | ||
| } |
There was a problem hiding this comment.
The new implementation is a great improvement and correctly handles numeric suffixes. However, there are a few areas where it could be made more robust and readable:
- Integer Overflow:
int counterandstart_numcould overflow if a block name has a very large numeric suffix, leading to undefined behavior. Usinglong longandstd::stollwould be safer. - Unhandled Exception:
std::stoican throwstd::out_of_rangeif the numeric suffix is too large to fit in anint, which would crash the program. This should be handled, for example with atry-catchblock. - Confusing Logic: The logic for initializing the counter,
int counter = (start_num > 0) ? start_num + 1 : 1;, is a bit subtle. While correct, its intent is not immediately obvious. A more explicit check would improve readability and maintainability.
Here is a suggested refactoring that addresses these points:
ffi::String GetUniqueName(const ffi::String& prefix) {
std::string str_prefix = std::string(prefix);
// Find where the trailing digits start
size_t base_len = str_prefix.length();
while (base_len > 0 && std::isdigit(str_prefix[base_len - 1])) {
--base_len;
}
std::string base_name;
long long start_num = 0;
bool has_suffix = base_len < str_prefix.length();
if (has_suffix) {
base_name = str_prefix.substr(0, base_len);
try {
start_num = std::stoll(str_prefix.substr(base_len));
} catch (const std::out_of_range&) {
// Fallback: if the number is too large, treat the whole string as a base name.
has_suffix = false;
base_name = str_prefix;
}
} else {
base_name = str_prefix;
}
// Check if the original name is available
ffi::String candidate = prefix;
if (!name_count_.count(candidate)) {
name_count_[candidate] = 0;
return candidate;
}
// Generate unique name by incrementing the numeric suffix
long long counter = has_suffix ? start_num + 1 : 1;
while (true) {
candidate = ffi::String(base_name + std::to_string(counter));
if (!name_count_.count(candidate)) {
name_count_[candidate] = 0;
return candidate;
}
++counter;
ICHECK_GT(counter, 0) << "Counter overflow when generating unique block name for prefix: "
<< prefix;
}
}
There was a problem hiding this comment.
I think this review makes sense especially the second. Could you modify the code like that?
There was a problem hiding this comment.
Make sense to me as well, I've updated it.
| ffi::String GetUniqueName(const ffi::String& prefix) { | ||
| ffi::String unique_prefix = prefix; | ||
| auto it = name_count_.find(prefix); | ||
| while (name_count_.count(unique_prefix)) { | ||
| unique_prefix = prefix + "_" + std::to_string(++it->second); | ||
| std::string str_prefix = std::string(prefix); | ||
|
|
||
| // Find where the trailing digits start | ||
| size_t base_len = str_prefix.length(); | ||
| while (base_len > 0 && std::isdigit(str_prefix[base_len - 1])) { | ||
| --base_len; | ||
| } | ||
|
|
||
| std::string base_name; | ||
| int start_num = 0; | ||
|
|
||
| if (base_len < str_prefix.length()) { | ||
| base_name = str_prefix.substr(0, base_len); | ||
| start_num = std::stoi(str_prefix.substr(base_len)); | ||
| } else { | ||
| base_name = str_prefix; | ||
| } | ||
|
|
||
| // Check if the original name is available | ||
| ffi::String candidate = prefix; | ||
| if (!name_count_.count(candidate)) { | ||
| name_count_[candidate] = 0; | ||
| return candidate; | ||
| } | ||
|
|
||
| // Generate unique name by incrementing the numeric suffix | ||
| int counter = (start_num > 0) ? start_num + 1 : 1; | ||
| while (true) { | ||
| candidate = ffi::String(base_name + std::to_string(counter)); | ||
| if (!name_count_.count(candidate)) { | ||
| name_count_[candidate] = 0; | ||
| return candidate; | ||
| } | ||
| ++counter; | ||
| } | ||
| name_count_[unique_prefix] = 0; | ||
| return unique_prefix; | ||
| } |
There was a problem hiding this comment.
I think this review makes sense especially the second. Could you modify the code like that?
|
Thanks! |
2 participants
Why
Resolve todo in
fuse_tir.ccby enhancing unique block name generation with numeric suffixes