(remove) doesn't work

(remove) doesn't work

[mskala]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 610 bytes --]

As far as I can tell, the (remove) form simply returns its second argument
instead of doing its job.  Test case attached - this will fail regardless
because of the lack of terrain types, but it'll also give an error about
trying to match a length-7 list with another list that should also be
length-7 but is in fact length-10.  The only shipped game module using
(remove) is tailhook.g.  I haven't figured out whether (remove) is
misbehaving in that file, but it definitely misbehaves in my test case.
-- 
Matthew Skala
mskala@ansuz.sooke.bc.ca                    Embrace and defend.
http://ansuz.sooke.bc.ca/

[-- Attachment #2: Type: TEXT/PLAIN, Size: 857 bytes --]

(game-module "remove"
  (title "Remove bug demo")
  (version "1.0")
  (blurb "Displays an error in the remove form.")
)

(unit-type unit1 (image-name "adventurer"))
(unit-type unit2 (image-name "adventurer"))
(unit-type unit3 (image-name "adventurer"))
(unit-type unit4 (image-name "adventurer"))
(unit-type unit5 (image-name "adventurer"))
(unit-type unit6 (image-name "adventurer"))
(unit-type unit7 (image-name "adventurer"))
(unit-type unit8 (image-name "adventurer"))
(unit-type unit9 (image-name "adventurer"))
(unit-type unit10 (image-name "adventurer"))

(define nonspecialunits (remove (unit1 unit2 unit3) u*))

;;;; Here's the error: the "remove" form should have set nonspecialunits to
;;;; a length-7 list (10 minus 3) but instead it will have set it to a
;;;; length-10 list.
(add nonspecialunits hp-max (1 2 3 4 5 6 7))

Re: (remove) doesn't work

[mskala]

On Tue, 18 May 2004 mskala@ansuz.sooke.bc.ca wrote:
> As far as I can tell, the (remove) form simply returns its second argument
> instead of doing its job.  Test case attached - this will fail regardless

Further to that: it seems it works if the first argument (the thing being
removed) is an atom.  You can do 10-1=9, but not 10-3=7.  Both my test
case and apparently tailhook.g depend on the documented behaviour of
removing a multi-element list from another multi-element list.
-- 
Matthew Skala
mskala@ansuz.sooke.bc.ca                    Embrace and defend.
http://ansuz.sooke.bc.ca/

Re: (remove) doesn't work

[Eric McDonald]

Hi Matthew,

On Tue, 2004-05-18 at 16:12, mskala@ansuz.sooke.bc.ca wrote:
> As far as I can tell, the (remove) form simply returns its second argument
> instead of doing its job.  Test case attached - this will fail regardless
> because of the lack of terrain types, but it'll also give an error about
> trying to match a length-7 list with another list that should also be
> length-7 but is in fact length-10.  The only shipped game module using
> (remove) is tailhook.g.  I haven't figured out whether (remove) is
> misbehaving in that file, but it definitely misbehaves in my test case.

I would definitely agree that 'remove' is broken, at least relative to
its documentation. I too tried to use it once, but it was not giving the
desired behavior, so I took a less convenient route that went around it.
I didn't mention anything at the time, because I thought it was just
some aspect of lispy behavior that I didn't fully comprehend (perhaps
this business about only being able to remove atoms rather than lists).

So, I guess the questions are, should it be able to remove lists? (It
would certainly be nice, since that is the way most people, myself
included, would want to use it) And, if so, how difficult would that
functionality be to add into the GDL parser? Hopefully someone will have
some insight into these questions, because otherwise I am going to have
to do some more "context gathering".

  Regards,
    Eric

Re: (remove) doesn't work

[mskala]

On Tue, 18 May 2004, Eric McDonald wrote:
> So, I guess the questions are, should it be able to remove lists? (It
> would certainly be nice, since that is the way most people, myself
> included, would want to use it) And, if so, how difficult would that
> functionality be to add into the GDL parser? Hopefully someone will have
> some insight into these questions, because otherwise I am going to have
> to do some more "context gathering".

It looks to me like it would be pretty easy to modify the eval() function
near line 1432 of lisp.c, where it calls remove_from_list(), to detect
when the thing being removed is a list, and in that case iterate over it.  
What concerns me is that this looks like the sort of thing that could have
wacky side effects if modified, because even though the code looks
straightforward right there, it seems to tie into LISP stuff that I don't
understand.
-- 
Matthew Skala
mskala@ansuz.sooke.bc.ca                    Embrace and defend.
http://ansuz.sooke.bc.ca/

Re: (remove) doesn't work

[Jim Kingdon]

> It looks to me like it would be pretty easy to modify the eval() function
> near line 1432 of lisp.c, where it calls remove_from_list(), to detect
> when the thing being removed is a list, and in that case iterate over it.  

Yeah.  The only thing that spring to my mind is whether xconq has
lists of lists, and whether you'd want

remove (c d) from (a b (c d) e) to return (a b e)

Off the top of my head I don't even remember whether xconq has lists
of lists, much less whether the above functionality is important.

> What concerns me is that this looks like the sort of thing that
> could have wacky side effects if modified, because even though the
> code looks straightforward right there, it seems to tie into LISP
> stuff that I don't understand.

I will point out that this kind of thing is quite easy to write unit
tests for.  Some of the existing stuff in autotest.c is strange, in
the sense of not cleanly clearing out the game state before each test
and getting things set up consistently.  But that issue of game state
isn't a problem for writing tests for lisp.c style code.

Although writing tests won't completely prevent unintended
consequences or bugs, it does make it easier to see what the code is
supposed to do and verify that it continues to work as it evolves.

Re: (remove) doesn't work

[mskala]

On Wed, 19 May 2004, Jim Kingdon wrote:
> Yeah.  The only thing that spring to my mind is whether xconq has
> lists of lists, and whether you'd want
> 
> remove (c d) from (a b (c d) e) to return (a b e)
> 
> Off the top of my head I don't even remember whether xconq has lists
> of lists, much less whether the above functionality is important.

I'm sure xconq has lists of lists, but it seems to me that if you want to
remove (c d) from (a b (c d) e) and return (a b e), you should use

  (remove ((c d)) (a b (c d) e))

not

  (remove (c d) (a b (c d) e)) .

-- 
Matthew Skala
mskala@ansuz.sooke.bc.ca                    Embrace and defend.
http://ansuz.sooke.bc.ca/

Re: (remove) doesn't work

[Eric McDonald]

On Wed, 2004-05-19 at 15:08, mskala@ansuz.sooke.bc.ca wrote:
> On Wed, 19 May 2004, Jim Kingdon wrote:
> > Yeah.  The only thing that spring to my mind is whether xconq has
> > lists of lists, and whether you'd want
> > 
> > remove (c d) from (a b (c d) e) to return (a b e)
> > 
> > Off the top of my head I don't even remember whether xconq has lists
> > of lists, much less whether the above functionality is important.
> 
> I'm sure xconq has lists of lists, but it seems to me that if you want to
> remove (c d) from (a b (c d) e) and return (a b e), you should use
> 
>   (remove ((c d)) (a b (c d) e))
> 
> not
> 
>   (remove (c d) (a b (c d) e)) .

I just looked at a Common Lisp reference, and it sides with Jim's
interpretation of the 'remove' keyword. Not that we have to follow Lisp
or anything....

Perhaps we could implement a separate 'remove-list' keyword, which would
follow your suggestion (and probably be more useful for GDL). If we
assume that order of atoms in the two lists need not match, then the
operation will effectively be O(n^2), which is probably not a big deal,
since it would only be used during game module interpretation.

Eric