STklos icon indicating copy to clipboard operation
STklos copied to clipboard

Simplify error checking

Open jpellegrini opened this issue 1 year ago • 4 comments

@egallesio this is mostly a matter of style. See if you agree...

In the compiler, instead of

  (if some-condition
      ( X ...
          ...
          ... )
      (compiler-error Y))

we do

  (unless some-condition (compiler-error Y))
  ( X ...
      ...
      ... )

Or when, when it makes sense

This makes error checking isolated in the place where it programatically happens, and also reduces source code size a little bit, making it clearer.

There are already uses of the macros when and unless in the compiler, so we're not introducing a dependency on them.

jpellegrini avatar Jun 22 '24 15:06 jpellegrini

I know that there are other places in STklos source that use the if style for error checking; I only changed the compiler, but I could in the future do the same in other files, gradually -- if you believe this is a good change.

jpellegrini avatar Jun 22 '24 17:06 jpellegrini

Hi @jpellegrini,

The form you propose is not equivalent to the original writing. In fact compiler-error calls error in interactive mode, bit it doesn't when we compile a file (it just prints an error message). For instance, in

(define (compile-quote expr env tail?)
  (unless (= (length expr) 2) (compiler-error 'quote expr "bad usage in ~S" expr))
  (compile-constant (cadr expr) env tail?))

If you call compile-quote with (quote), you'll access the cadr of a list of length 1, which will abort the compilation of the rest of the file.

egallesio avatar Jun 24 '24 12:06 egallesio

Oh, I saw that the new form wasn't equivalent, but I thought it wouldn't be relevant, because it passed all tests. Maybe we need more tests then! :grin:

jpellegrini avatar Jun 24 '24 16:06 jpellegrini

Oh, I saw that the new form wasn't equivalent, but I thought it wouldn't be relevant, because it passed all tests. Maybe we need more tests then! 😁

The problem here is that the tests (in make test) are not compiled, but just loaded.

egallesio avatar Jul 01 '24 14:07 egallesio

So since the benefits would be minimal, and this introduces problems, I believe it would be OK to close this one?

jpellegrini avatar Jun 19 '25 13:06 jpellegrini

Yep, you are right. I'm closing it.

egallesio avatar Jun 19 '25 15:06 egallesio