gemini-code-assist[bot] commented on code in PR #18554:
URL: https://github.com/apache/tvm/pull/18554#discussion_r2594489312
##########
src/relax/transform/fuse_tir.cc:
##########
@@ -357,17 +357,43 @@ class BlockNameDeduplicator : public tir::StmtMutator {
}
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;
}
Review 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:
1. **Integer Overflow**: `int counter` and `start_num` could overflow if a
block name has a very large numeric suffix, leading to undefined behavior.
Using `long long` and `std::stoll` would be safer.
2. **Unhandled Exception**: `std::stoi` can throw `std::out_of_range` if
the numeric suffix is too large to fit in an `int`, which would crash the
program. This should be handled, for example with a `try-catch` block.
3. **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:
```c
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;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
