Re: Optimizations for flymake

[storm@cua.dk (Kim F. Storm)]

Stefan <monnier@iro.umontreal.ca> writes:

>> ! (defsubst flymake-makehash(&optional test)
>> !   (if (featurep 'xemacs)
>> !       (if test (make-hash-table :test test) (make-hash-table))
>> !     (makehash test)))
>
> Why not (if (fboundp 'make-hash-table) ...) ?

Right, that's better.

>   
>>   (defun flymake-float-time()
>> !     (if (featurep 'xemacs)
>> ! 	(let ((tm (current-time)))
>> ! 	  (multiple-value-bind (s0 s1 s2) (current-time)
>> ! 			       (+ (* (float (ash 1 16)) s0) (float s1) (* 0.0000001 s2))))
>> !       (float-time)))
>
> Why not (if (fboundp 'float-time) ...) ?
>

Because Emacs doesn't have multiple-value-bind.

>> ! (defsubst flymake-replace-regexp-in-string(regexp rep str)
>> !     (if (featurep 'xemacs)
>> ! 	(replace-in-string str regexp rep)
>> !       (replace-regexp-in-string regexp rep str)))
>
> Why not (if (fboundp 'replace-regexp-in-string) ...) ?
> I'd actually make it even more efficient than `defsubst':

Like this?

(defalias 'flymake-replace-regexp-in-string
  (if (fboundp 'replace-regexp-in-string))
      'replace-regexp-in-string
    'replace-in-string))


>
>> ! (defsubst flymake-split-string(str pattern)
>> !     (if (featurep 'xemacs)
>> ! 	(flymake-split-string-remove-empty-edges str pattern)
>> !       (flymake-split-string-remove-empty-edges str pattern)))
>
> Hmmm... isn't this just
> (defalias 'flymake-split-string 'flymake-split-string-remove-empty-edges) ?

Yes -- looks odd.

>
>> ! (defsubst flymake-get-temp-dir()
>> !     (if (featurep 'xemacs)
>> ! 	(temp-directory)
>> !       temporary-file-directory))
>
> Why not (if (fboundp 'temp-directory) ...) ?

Would work as well -- but it would hide why we make the check...

>
>>   (defun flymake-popup-menu(pos menu-data)
>> !     (if (featurep 'xemacs)
>> ! 	(let* ((x-pos       (nth 0 (nth 0 pos)))
>> ! 	       (y-pos       (nth 1 (nth 0 pos)))
>> ! 	       (fake-event-props  '(button 1 x 1 y 1)))
>> ! 	  (setq fake-event-props (plist-put fake-event-props 'x x-pos))
>> ! 	  (setq fake-event-props (plist-put fake-event-props 'y y-pos))
>> ! 	  (popup-menu (flymake-make-xemacs-menu menu-data) (make-event 'button-press fake-event-props))
>> ! 	  )
>> !       (x-popup-menu pos (flymake-make-emacs-menu menu-data))))
>
> Since Emacs-21 has popup-menu, we should be able to throw away
> flymake-make-emacs-menu and rename flymake-make-xemacs-menu to
> flymake-make-menu:
>   
> (defun flymake-popup-menu(pos menu-data)
>   (popup-menu (flymake-make-menu menu-data)
>               (if (not (featurep 'xemacs))
> 	          pos
>                 (let* ((x-pos       (nth 0 (nth 0 pos)))
> 	               (y-pos       (nth 1 (nth 0 pos)))
> 	               (fake-event-props  '(button 1 x 1 y 1)))
> 	          (setq fake-event-props (plist-put fake-event-props 'x x-pos))
> 	          (setq fake-event-props (plist-put fake-event-props 'y y-pos))
> 	          (make-event 'button-press fake-event-props)))))

I take your word for it :-)

>
>>   (defun flymake-current-row()
>>       "return current row in current frame"
>> !     (if (featurep 'xemacs)
>> ! 	(count-lines (window-start) (point))
>> !       (+ (car (cdr (window-edges))) (count-lines (window-start) (point)))))
>
> I suspect this should be:
>
> (defun flymake-current-row()
>   "return current row in current frame"
>   (+ (count-lines (window-start) (point))
>      (if (fboundp 'window-edges)
>          (car (cdr (window-edges)))
>        ;; On XEmacs we should probably use something else, but what??
>        0)))

Ok.

>
>> ! (defsubst flymake-selected-frame()
>> !     (if (featurep 'xemacs)
>> !         (selected-window)
>> !       (selected-frame)))
>
> XEmacs has `selected-frame' as well, so the above code looks odd.
> I must be missing something.

Me too.


It also occurred to me that rather than all the autoloads for the
various overlay things, it should simply do:

(require 'overlay)

Emacs has (featurep 'overlay) => t, so there's no need to
condition it with (featurep 'xemacs) either.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk