Skip to content

Conversation

iory
Copy link
Contributor

@iory iory commented Jul 3, 2025

This pull request addresses a minor bug in the :find method for packages and significantly improves its error reporting for easier debugging.

  • Fixed Incorrect Variable in Error Message: The error message previously used the sym variable, which could display the wrong symbol when a search failed. This has been corrected to use s, ensuring the message always shows the symbol that was actually being searched for.

  • Enhanced Error Details: The error message has been expanded to be more informative. It now includes the search limit and the hash table's size, providing valuable context that can help diagnose a full hash table more quickly.

Quick check

Check code

  (dotimes (i 100)
    (eval `(defconstant ,(intern (format nil "*CONST~d*" i)) i)))
  (make-package "TEST1")
  (dotimes (i (+ (length ((find-package "TEST1") . intsymvector)) 10))
    (eval `(defconstant ,(intern (format nil "TEST1::*CONST~d*" i)) i)))
  (make-package "TEST2")
  (dotimes (i (+ (length ((find-package "TEST2") . intsymvector)) 10))
    (print i)
    (catch 'error
      (labels ((error2 (&rest args) (print args *error-output*)(throw 'error nil)))
              (lisp::install-error-handler 'error2)
              (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2"))
              (eval `(defconstant ,(intern (format nil "TEST2::*CONST~d*" i)) i)))))

Without this PR,

0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
Call Stack (max depth: 20):
  0: at (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2"))
  1: at (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))
  2: at (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i))))))
  3: at (while (< i #:dotimes64) (print i) (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))) (setq i (1+ i)))
  4: at (let ((i 0) (#:dotimes64 (+ (length ((find-package "TEST2") . intsymvector)) 10))) (declare (integer i #:dotimes64)) (while (< i #:dotimes64) (print i) (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))) (setq i (1+ i))) nil)
  5: at (dotimes (i (+ (length ((find-package "TEST2") . intsymvector)) 10)) (print i) (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))))
  6: at #<compiled-code #X15049b380>
(76 "" (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) "can not find test2::*const27* in this package")

In this case test2::*const27* is not target symbol.

With this PR

0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
Call Stack (max depth: 20):
  0: at (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2"))
  1: at (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))
  2: at (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i))))))
  3: at (while (< i #:dotimes64) (print i) (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))) (setq i (1+ i)))
  4: at (let ((i 0) (#:dotimes64 (+ (length ((find-package "TEST2") . intsymvector)) 10))) (declare (integer i #:dotimes64)) (while (< i #:dotimes64) (print i) (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))) (setq i (1+ i))) nil)
  5: at (dotimes (i (+ (length ((find-package "TEST2") . intsymvector)) 10)) (print i) (catch 'error (labels ((error2 (&rest args) (print args *error-output*) (throw 'error nil))) (lisp::install-error-handler 'error2) (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) (eval (cons 'defconstant (cons (intern (format nil "TEST2::*CONST~d*" i)) (list 'i)))))))
  6: at #<compiled-code #X138010980>
(76 "" (shadow (intern (format nil "*CONST~d*" i)) (find-package "TEST2")) "Cannot find symbol *const60*. Search limit reached (120 slots). Hash table (size: 60) may be full.")

In this case test2::*const60* is target symbol and the program outputs correct error message.

@iory iory requested a review from Copilot July 3, 2025 04:38
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in the :find method by using the correct variable (s instead of sym) in the error message and enhances the error to include both the search limit and hash table size for better diagnostics.

  • Switched the error call to reference the actual searched symbol (s)
  • Expanded the error message with search limit (2*size) and hash table size (size)
Comments suppressed due to low confidence (2)

lisp/l/packsym.l:169

  • Add a unit test that triggers this error path to assert the message correctly includes the search limit (2*size) and the hash table size (size).
      (error "Cannot find symbol ~s. Search limit reached (~d slots). Hash table (size: ~d) may be full."

lisp/l/packsym.l:170

  • [nitpick] The single-letter variable s is ambiguous; consider renaming it to something like search-symbol or target-symbol to improve readability.
           s (* 2 size) size)))

Copy link
Member

@k-okada k-okada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM

@k-okada k-okada merged commit 71a2528 into euslisp:master Jul 3, 2025
102 of 104 checks passed
@iory iory deleted the fix-find-symbol branch July 3, 2025 07:26
k-okada added a commit that referenced this pull request Sep 25, 2025
- apply patches in dfsg https://salsa.debian.org/science-team/euslisp/-/tree/debian/9.31+dfsg-1/debian/patches?ref_type=tags
	- On arm32, localtime_r in LOCALTIME in unixcall.c returns NULL in some situation (#524)
	- Fix for blhf Fix for blhc tests, we need to use default CFLAGS (#523)
	- defforeign on ppc64el is not work for float/double, skpping test for now (#525)
	- update VERSION=9.31 (#517)
	- remove link to libeus from executables, this also remove EUSLIB, which provides RPATH settings that Debian dislike. (#522)
	- use same code for every build (only common.l)  (#522)
	- Fix hardening-no-bindnow by adding DEB_BUILD_MAINT_OPTIONS = hardening=+all to rule and pass that variable to CFLAGS/SOFLAGS/LDFLAGS, use gcc for LD.  (#522)
- use termios.h, termio.h is dropped for glibc >= 2.42, fix lisp/tool/eustags.c for gcc^15 (#533)
- Fixed find symbol function by using 's' instead of 'sym' variable and added hash table size (#532)
- Support apple silicon arm architecture. Updates C code to handle 64-bit pointers correctly, especially for printing memory addresses. (#529)
- make sure to run MKDIR before compile GCCLS (#527)
- update code for gcc-15 (#521)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants