Cleaning up code

Cleaning up code

[Stefan Monnier]

If you feel like cleaning up some code, but are not sure where to look,
try the patch below and then bootstrap Emacs.  That should give you
enough warnings to keep you busy for a while.


        Stefan


=== modified file 'lisp/Makefile.in'
--- lisp/Makefile.in	2013-07-23 22:55:38 +0000
+++ lisp/Makefile.in	2013-08-05 22:00:36 +0000
@@ -42,7 +42,8 @@
 EMACSOPT = -batch --no-site-file --no-site-lisp
 
 # Extra flags to pass to the byte compiler
-BYTE_COMPILE_EXTRA_FLAGS =
+BYTE_COMPILE_EXTRA_FLAGS = --eval '(setq byte-compile-force-lexical-warnings t)'
+
 # For example to not display the undefined function warnings you can use this:
 # BYTE_COMPILE_EXTRA_FLAGS = --eval '(setq byte-compile-warnings (quote (not unresolved)))'
 # The example above is just for developers, it should not be used by default.

Re: Cleaning up code

[Michael Albinus]

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

> If you feel like cleaning up some code, but are not sure where to look,
> try the patch below and then bootstrap Emacs.  That should give you
> enough warnings to keep you busy for a while.

Yay!

However, setting `byte-compile-force-lexical-warnings' to t is too
coarse. For example, Tramp's macro `with-parsed-tramp-file-name'
let-binds several variables "just in case". They are reported as unused
then, on every invocation of that macro.

Is there a way to tell the byte compiler, that these unused declarations
are intended here? Otherwise, I would be swamped with warnings, and I
won't see the real warnings I should care.

>         Stefan

Best regards, Michael.

Re: Cleaning up code

[Stefan Monnier]

> However, setting `byte-compile-force-lexical-warnings' to t is too
> coarse. For example, Tramp's macro `with-parsed-tramp-file-name'
> let-binds several variables "just in case". They are reported as unused
> then, on every invocation of that macro.

Yup, that's one of the main problems: when a single let in the source
ends up duplicated after expansion so some expansions may use it while
others end up not using it.

We need to add some way to tell Emacs that it should check "the sum of
all uses" or something like that.  In your case, this "sum" is
open-ended, so we should instead just tell it not to check at all.


        Stefan

Re: Cleaning up code

[Michael Albinus]

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

>> However, setting `byte-compile-force-lexical-warnings' to t is too
>> coarse. For example, Tramp's macro `with-parsed-tramp-file-name'
>> let-binds several variables "just in case". They are reported as unused
>> then, on every invocation of that macro.
>
> Yup, that's one of the main problems: when a single let in the source
> ends up duplicated after expansion so some expansions may use it while
> others end up not using it.
>
> We need to add some way to tell Emacs that it should check "the sum of
> all uses" or something like that.  In your case, this "sum" is
> open-ended, so we should instead just tell it not to check at all.

Yep.

There is also another case where it doesn't work as expected. Often, I
let bind a variable for side effect, like this:

      (let ((default-directory (tramp-compat-temporary-file-directory))
            (outline-regexp tramp-debug-outline-regexp))
        (outline-mode))

And I get from the byte compiler

tramp.el:1400:1:Warning: Unused lexical variable `outline-regexp'

In Tramp, there are many such stanzas of intended side effect
bindings. Therefore, there are too many false positives, that I could
see the real warnings I should fix.

Could we have a byte-compiler option telling that such bindings are
intended? Something like this:

      (let ((byte-compile-force-lexical-warnings nil)
            (default-directory (tramp-compat-temporary-file-directory))
            (outline-regexp tramp-debug-outline-regexp))
        (outline-mode))

>         Stefan

Best regards, Michael.

Re: Cleaning up code

[Stefan Monnier]

> There is also another case where it doesn't work as expected. Often, I
> let bind a variable for side effect, like this:

>       (let ((default-directory (tramp-compat-temporary-file-directory))
>             (outline-regexp tramp-debug-outline-regexp))
>         (outline-mode))

> And I get from the byte compiler

> tramp.el:1400:1:Warning: Unused lexical variable `outline-regexp'

That is a correct warning, telling you that it treated outline-regexp as
a lexical variable and hence that it did not generate the code you want.
IOW you need to add

   (defvar outline-regexp)


-- Stefan

Re: Cleaning up code

[Michael Albinus]

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

> IOW you need to add
>
>    (defvar outline-regexp)

Thanks. By this, I could remove some other byte compiler warnings.

> -- Stefan

Best regards, Michael.