Re: Compilation warnings of ELisp seem wrong and misleading

[Pascal J. Bourguignon]

"Ludwig, Mark" <ludwig.mark@siemens.com> writes:

> "Pascal J. Bourguignon" wrote:
>
>> "Ludwig, Mark" <ludwig.mark@siemens.com> writes:
>> 
>> > Greetings,
>> >
>> > I've been using an ancient Emacs (19.29) on Solaris and finally
>> > got around to installing a current one (24.4).
>> >
>> > I find my custom Elisp generates warnings that seem pretty stupid.
>> > For example:
>> >
>> > emacs.el:255:10:Warning: reference to free variable `if'
>> > emacs.el:219:8:Warning: reference to free variable `save-excursion'
>> > emacs.el:331:41:Warning: reference to free variable `forward-char'
>> > emacs.el:261:17:Warning: reference to free variable `insert'
>> > emacs.el:261:17:Warning: reference to free variable `forward-sexp'
>> >
>> > Those are all valid functions.  For example, here are lines 255-258:
>> >
>> >    (if (not (= ans ?q))
>> >        (progn
>> >          (goto-char found-start)
>> >          (delete-region found-start found-end)))
>> >
>> > This is inside a large-ish "let*" form (111 lines).
>
>> We cannot see either, because the meaning of a sexp is determined by its
>> surrounding form, which you didn't provide.
>> 
>> You probably have a parenthesis problem in your let* form which makes
>> lisp interpret some parts as being variable names instead of operators.
>
> No, I know how to balance parentheses.  I've been
> programming Emacs Lisp for almost 30 years.  I rely on Emacs
> for its outstanding job of showing structural problems
> through its automatic indentation (in all programming
> languages, but certainly within its own).  By all means, if
> there's anything wrong with that (if ...) form, please
> explain.
>
> For context, the "let*" is on line 239; the warning
> (mentioned above as being on line 255 column 10) is
> referring to the form after the comment "Make the
> substitution."  If you read the warnings carefully, you'll
> note that it warns about two different things on line 261,
> column 17.  That line is the one with the comment, 'Now it's
> "strlcpy" or whatever.'  Clearly, the byte-compiler is
> pretty confused....


No it is not confused.  
The warning are correct.  
However, it reports them at seemingly wrong places.

First, the code you gave has indeed 5 free variables.  It should not
compile and it should not run!  I had to add them to be able to compile
it, in a surrounding let.

Second, the code you gave is ill-formatted.  How hard is it to put the
closing parentheses on the same line (try to avoid ;-comments on the
last expression before a closing parenthesis), and how hard is it to
type M-q to have emacs indent your code correctly?!?

Next, the warning reported:

Compiling file /tmp/e.el at Mon Mar 30 18:38:30 2015
e.el:65:14:Warning: reference to free variable `if'
e.el:90:33:Warning: reference to free variable `save-excursion'
e.el:99:33:Warning: reference to free variable `forward-char'
e.el:32:12:Warning: reference to free variable `insert'
e.el:32:12:Warning: reference to free variable `forward-sexp'
e.el:110:33:Warning: reference to free variable `c-indent-exp'

correspond indeed to those symbols used in variable position, as cond
expressions!  Which can be clearly seen, when you have your code
indented properly, starting from line 85 below:


     1  (let ((from  "from")
     2        (to1   "T")
     3        (to2   "t")
     4        (found-start 0)
     5        (found-end   2))
     6   (let* ((c1 (string-to-char to1))
     7          (c2 (string-to-char to2))
     8          (ans 0))
     9  
    10     ;; Get one of the first characters from the two substitutions
    11  
    12     (while (and (not (= ans c1))
    13                 (not (= ans c2))
    14                 (not (= ans ?q)))     ; Quit, don't substitute anything
    15       (setq ans (read-char-exclusive
    16                  (concat "Replace " from " with "
    17                          to1 " (will truncate silently) or "
    18                          to2 " (dstat)? (" (char-to-string c1) "/" 
(char-to-string c2) "/q) "))))
    19  
    20     ;; Make the substitution
    21  
    22     (if (not (= ans ?q))
    23         (progn
    24           (goto-char found-start)
    25           (delete-region found-start found-end)))
    26  
    27     (cond ((= ans c1)
    28            (insert to1)                ; Now it's "strlcpy" or whatever
    29            )
    30  
    31           ((= ans c2)
    32            (insert "dstat = " to2) ; Now it's "dstat = nlsStrLCpy" or 
whatever
    33            (back-to-indentation)
    34            (setq ans 0)
    35            (while (and (not (= ans ?d))
    36                        (not (= ans ?e))
    37                        (not (= ans ?n)))
    38              (setq ans (read-char-exclusive "Use DSOK/EXIT/no form? ")))
    39  
    40            (let ((if-pos (point)))
    41  
    42              (cond ((= ans ?d)
    43                     (insert "if (DSOK) ")
    44                     (forward-sexp 3) ; After the closing parenthesis for 
the function call
    45                     )
    46  
    47                    ((= ans ?e)
    48                     (insert "if (")
    49                     (forward-sexp 3) ; After the closing parenthesis for 
the function call
    50                     (insert ")")
    51                     (newline-and-indent)
    52                     (insert "goto EXIT") ; The semi-colon should end up 
after "EXIT"
    53                     (c-indent-command) ; Make the "goto" be correctly 
indented under the "if"
    54                     )
    55  
    56                    ((= ans ?n)
    57                     ;; no form
    58                     ))
    59  
    60              ;; Try to look ahead, in case the function call we just
    61              ;; modified was a one-statement "block" in an "if," so
    62              ;; we've just unintentionally modified the meaning of
    63              ;; that "if" statement
    64             
    65              (if (not (= ans ?n))
    66                  (condition-case
    67                   nil
    68                   (progn
    69                     (forward-sexp 1)   ; Skip over "else" if it's there
    70                     (forward-sexp -1) ; Back in front of "else" if it's 
there
    71                     (if (looking-at "else")
    72                         (progn
    73                           (goto-char if-pos) ; Go back to where we 
inserted "if"
    74  
    75                           (cond ((= ans ?d)
    76                                  ;; Skip over: "if" "(DSOK)" "dstat" "= 
..." "()"
    77                                  (forward-sexp 5)
    78                                  )
    79  
    80                                 ((= ans ?e)
    81                                  ;; Skip over:
    82                                  ;; "if" "(dstat = ...)" "goto" "EXIT"
    83                                  (forward-sexp 4))
    84  
    85                                 (if (not (looking-at ";"))
    86                                     (error "First expected ';' not: 
\"%s\""
    87                                            (buffer-substring (point)
    88                                                              (progn 
(end-of-line 1)
    89                                                                     
(point)))))
    90                                 (save-excursion
    91                                  (goto-char if-pos) ; Go back to where 
we inserted "if"
    92                                  (insert "{
    93  "))
    94                                 (if (not (looking-at ";"))
    95                                     (error "Second expected ';' not: 
\"%s\""
    96                                            (buffer-substring (point)
    97                                                              (progn 
(end-of-line 1)
    98                                                                     
(point)))))
    99                                 (forward-char 1)
   100  
   101                                 ;; We should be looking at the end of 
line
   102                                 (if (not (looking-at "\\s-*$"))
   103                                     (error "Second expected end of line, 
not: \"%s\""
   104                                            (buffer-substring (point)
   105                                                              (progn 
(end-of-line 1)
   106                                                                     
(point)))))
   107                                 (insert "
   108  }")
   109                                 (forward-sexp -1)
   110                                 (c-indent-exp)
   111                                 ))))
   112                   (error nil)          ;hand-rolled (ignore-errors ...)
   113                   )))))))


You may indeed report an emacs bug, that the warnings refer the first
occurences of the symbol, instead of the erroneous occurences.


Finally, you should have expressions so long. You should write short
functions, and use them in shorter forms.


Your code should be replaced with a single simple function:

(defun do-something (from to1 to2 found-start found-end)
  (let* ((c1 (string-to-char to1))
         (c2 (string-to-char to2))
         (ans (get-one-of-the-first-characters-from-the-two-substitutions from 
to1 to2 c1 c2)))
    (unless (= ans ?q)
      (make-the-substitution ans from to1 to2 c1 c2 found-start found-end))))


;; which uses the two simple and clear functions:

(defun get-one-of-the-first-characters-from-the-two-substitutions (from to1 to2 
c1 c2)
  (let ((ans 0))
    (while (and (/= ans c1)
                (/= ans c2)
                (/= ans ?q))          ; Quit, don't substitute anything
      (setq ans (read-char-exclusive
                 (concat "Replace " from " with "
                         to1 " (will truncate silently) or "
                         to2 " (dstat)? (" (char-to-string c1) "/" 
(char-to-string c2) "/q) "))))
    ans))


(defun make-the-substitution (ans from to1 to2 c1 c2 found-start found-end)
  (assert (/= ans ?q))
  (goto-char found-start)
  (delete-region found-start found-end)
  (cond ((= ans c1) (insert to1))
        ((= ans c2) (insert-dstat ans from to2 c2 found-start found-end))))


;; Also notice how they can very easily be tested alone:

;; (get-one-of-the-first-characters-from-the-two-substitutions "from" "T" "t" 
?T ?t)
;; (make-the-substitution ?T "from" "T" "t" ?T ?t (point) (point))


;; With make-the-substitution itself using the following functions:

(defun get-dstat-choice ()
  (let ((ans 0))
    (while (and (/= ans ?d)
                (/= ans ?e)
                (/= ans ?n))
      (setq ans (read-char-exclusive "Use DSOK/EXIT/no form? ")))
    ans))

;; again, very easily testable: (get-dstat-choice)


(defun insert-dstat (ans from to2 c2 found-start found-end)
  (insert "dstat = " to2) ; Now it's "dstat = nlsStrLCpy" or whatever
  (back-to-indentation)
  (let ((ans    (get-dstat-choice))
        (if-pos (point)))
    (unless (= ans ?n)

      (case ans
        (?d
         (insert "if (DSOK) ")
         (forward-sexp 3))
        (?e
         (insert "if (")
         (forward-sexp 3) ; After the closing parenthesis for the function call
         (insert ")")
         (newline-and-indent)
         (insert "goto EXIT") ; The semi-colon should end up after "EXIT"
         (c-indent-command)))

      ;; Try to look ahead, in case the function call we just
      ;; modified was a one-statement "block" in an "if," so
      ;; we've just unintentionally modified the meaning of
      ;; that "if" statement
      
      (forward-sexp 1)  ; Skip over "else" if it's there
      (forward-sexp -1) ; Back in front of "else" if it's there
      (when (looking-at "else")
        (goto-char if-pos) ; Go back to where we inserted "if"
        (case ans
          (?d ;; Skip over: "if" "(DSOK)" "dstat" "= ..." "()"
           (forward-sexp 5))
          (?e
           ;; Skip over:
           ;; "if" "(dstat = ...)" "goto" "EXIT"
           (forward-sexp 4))
          (otherwise ;; perhaps you didn't mean otherwise, but end of
                     ;; the case?  You'll have to debug it here.
           (unless (looking-at ";")
             (error "First expected ';' not: \"%s\""
                    (buffer-substring (point)
                                      (progn (end-of-line 1)
                                             (point)))))
           (save-excursion
            (goto-char if-pos) ; Go back to where we inserted "if"
            (insert "{\n"))
           (unless (looking-at ";")
             (error "Second expected ';' not: \"%s\""
                    (buffer-substring (point)
                                      (progn (end-of-line 1)
                                             (point)))))
           (forward-char 1)
           ;; We should be looking at the end of line
           (unless  (looking-at "\\s-*$")
             (error "Second expected end of line, not: \"%s\""
                    (buffer-substring (point)
                                      (progn (end-of-line 1)
                                             (point)))))
           (insert "\n}")
           (forward-sexp -1)
           (c-indent-exp)))))))


;; you an then call your functions from the toplevel:

(let ((from  "from")
      (to1   "T")
      (to2   "t")
      (found-start 0)
      (found-end   2))
  (do-something from to1 to2 found-start found-end))



-- 
__Pascal Bourguignon__                 http://www.informatimago.com/
“The factory of the future will have only two employees, a man and a
dog. The man will be there to feed the dog. The dog will be there to
keep the man from touching the equipment.” -- Carl Bass CEO Autodesk