gemini-code-assist[bot] commented on code in PR #19916:
URL: https://github.com/apache/tvm/pull/19916#discussion_r3498690434
##########
src/script/printer/doc_printer/base_doc_printer.cc:
##########
@@ -267,6 +267,7 @@ void DocPrinter::Append(const Doc& doc, const
PrinterConfig& cfg) {
for (const AccessPath& p : cfg->path_to_underline) {
path_to_underline_.push_back(p);
current_max_path_depth_.push_back(0);
+ current_visible_paths_.push_back(std::nullopt);
current_underline_candidates_.push_back(std::vector<ByteSpan>());
}
Review Comment:
If `cfg->path_to_underline` is null or undefined, iterating over it directly
will cause a segmentation fault or undefined behavior. We should guard the loop
with a `.defined()` check to ensure safety.
```c
if (cfg->path_to_underline.defined()) {
for (const AccessPath& p : cfg->path_to_underline) {
path_to_underline_.push_back(p);
current_max_path_depth_.push_back(0);
current_visible_paths_.push_back(std::nullopt);
current_underline_candidates_.push_back(std::vector<ByteSpan>());
}
}
```
##########
tests/python/tvmscript/test_tvmscript_printer_structural_equal.py:
##########
@@ -29,11 +29,58 @@ def _error_message(exception):
return str(exception)
+def _access_step_text(step):
+ kind = AccessKind(step.kind)
+ if kind == AccessKind.ATTR:
+ return f".{step.key}"
+ if kind == AccessKind.ARRAY_ITEM:
+ return f"[{step.key}]"
+ if kind == AccessKind.MAP_ITEM:
+ return f"[{step.key!r}]"
+ if kind == AccessKind.ATTR_MISSING:
+ return f"[<missing:{step.key!r}>]"
+ if kind == AccessKind.ARRAY_ITEM_MISSING:
+ return f"[<missing:{step.key}>]"
+ if kind == AccessKind.MAP_ITEM_MISSING:
+ return f"[<missing:{step.key!r}>]"
+ raise ValueError(f"Unknown AccessKind: {kind}")
+
+
+def _hidden_path_suffix(requested_path, visible_path):
+ return "".join(
+ _access_step_text(step) for step in
requested_path.to_steps()[visible_path.depth :]
+ )
+
+
+def _location_block(obj, objpath):
+ script, visible_paths = obj.script(
+ path_to_underline=[objpath],
+ syntax_sugar=False,
+ render_invisible_path_info=True,
+ )
+ visible_path = visible_paths[0] if visible_paths else None
+ lines = [f"Access path: {objpath}"]
+ if visible_path is not None and visible_path != objpath and
visible_path.is_prefix_of(objpath):
+ lines.extend(
+ [
+ f"Highlighted object: {visible_path}",
+ f"Hidden field: {_hidden_path_suffix(objpath, visible_path)}",
+ "Note: The hidden field is not rendered in TVMScript, so the
underline "
+ "points to the nearest visible object in the access path.",
+ ]
+ )
+ context = "\n".join(lines)
+ return f"{context}\n\n{script}"
Review Comment:
The helper functions `_access_step_text`, `_hidden_path_suffix`, and the
manual formatting in `_location_block` are completely duplicated from the
production implementation in `tvm.runtime.script_printer`. We can simplify this
significantly and improve maintainability by importing and reusing
`_script_with_invisible_path_info` directly.
```suggestion
def _location_block(obj, objpath):
from tvm.runtime.script_printer import _script_with_invisible_path_info,
PrinterConfig
return _script_with_invisible_path_info(
obj,
PrinterConfig(
syntax_sugar=False,
path_to_underline=[objpath],
),
objpath,
)
```
##########
src/script/printer/script_printer.cc:
##########
@@ -23,8 +23,36 @@
#include <tvm/script/printer/printer.h>
#include <algorithm>
+#include <optional>
+
+#include "visible_path.h"
namespace tvm {
+namespace {
+
+using AccessPath = ffi::reflection::AccessPath;
+
+ffi::Array<ffi::Optional<AccessPath>> EmptyVisiblePaths(const PrinterConfig&
cfg) {
+ ffi::Array<ffi::Optional<AccessPath>> result;
+ for (size_t i = 0; i < cfg->path_to_underline.size(); ++i) {
+ result.push_back(std::nullopt);
+ }
+ return result;
+}
Review Comment:
If `cfg->path_to_underline` is null or undefined, calling `.size()` on it
will cause a segmentation fault. We should guard the loop with a `.defined()`
check to ensure safety.
```c
ffi::Array<ffi::Optional<AccessPath>> EmptyVisiblePaths(const PrinterConfig&
cfg) {
ffi::Array<ffi::Optional<AccessPath>> result;
if (cfg->path_to_underline.defined()) {
for (size_t i = 0; i < cfg->path_to_underline.size(); ++i) {
result.push_back(std::nullopt);
}
}
return result;
}
```
--
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]
