Conversation
Greptile SummaryThis PR enhances the "unbound variable" error experience in Goldfish by scanning Confidence Score: 5/5Safe to merge; remaining findings are P2 style/cleanup suggestions that don't affect correctness. All logic is correct and well-tested. The two flagged items (redundant loop iteration on the operator and manual port cleanup) are both P2 style concerns with no observable impact on behaviour in the current S7 configuration. src/goldfish.hpp — two minor style notes in the new helper functions. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Scheme error captured\nto error port] --> B[goldfish_render_scheme_error_message]
B --> C[goldfish_format_scheme_error_message\nformat raw error text]
C --> D[goldfish_append_doc_hint_if_needed]
D --> E{contains\n'unbound variable'?}
E -- No --> F[return as-is]
E -- Yes --> G[goldfish_extract_unbound_function_name_from_error\nextract symbol name]
G --> H{direct call?\nin open-paren form}
H -- Yes direct --> I[symbol extracted]
H -- No --> J[goldfish_extract_error_expression\nparse error context]
J --> K[goldfish_error_expression_contains_function_call\nAST check via s7_read]
K -- found in call position --> I
K -- not found --> F
I --> L[find_function_libraries_in_load_path\nscan *load-path* .scm files]
L --> M{how many\nlibraries found?}
M -- 0 --> N["Hint: try gf doc fn\n+ git grep suggestion"]
M -- 1 --> O["Hint: fn exists in lib\nPlease import lib"]
M -- many --> P["Hint: fn exists in multiple libs\nTry: gf doc lib fn for each"]
Reviews (1): Last reviewed commit: "wip" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (s7_pointer iter= form; s7_is_pair (iter); iter= s7_cdr (iter)) { | ||
| if (goldfish_form_contains_called_symbol (sc, s7_car (iter), function_name)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The for loop starts from iter = form instead of s7_cdr(form), so s7_car(form) (the operator) is visited twice: once by the explicit symbol check just above, and once by the loop's first iteration. The recursive call on a symbol always returns false (symbols aren't pairs), so this doesn't produce wrong results, but it is a minor unnecessary traversal.
| for (s7_pointer iter= form; s7_is_pair (iter); iter= s7_cdr (iter)) { | |
| if (goldfish_form_contains_called_symbol (sc, s7_car (iter), function_name)) { | |
| return true; | |
| } | |
| } | |
| for (s7_pointer iter= s7_cdr (form); s7_is_pair (iter); iter= s7_cdr (iter)) { | |
| if (goldfish_form_contains_called_symbol (sc, s7_car (iter), function_name)) { | |
| return true; | |
| } | |
| } |
| string source_text= read_text_file_exact (source_file); | ||
| s7_pointer port = s7_open_input_string (sc, source_text.c_str ()); | ||
| s7_pointer eof_object = s7_eof_object (sc); | ||
|
|
||
| while (true) { | ||
| s7_pointer form= s7_read (sc, port); | ||
| if (form == eof_object) break; | ||
| if (define_library_form_exports_function (sc, form, function_name, group, library)) { |
There was a problem hiding this comment.
source_file_exports_function
The port opened by s7_open_input_string is closed manually on both return true and return false paths, but if s7_read or define_library_form_exports_function causes a non-local exit (e.g., S7's internal error handling in some build configurations), s7_close_input_port will be skipped. S7 string ports are GC-managed, so this is unlikely to leak OS resources, but a RAII wrapper or a local guard object would make the lifetime management explicit and safe.
2 participants
No description provided.