View Issue Details

IDProjectCategoryView StatusLast Update
0000519LDMud 3.3Efunspublic2009-01-03 12:04
Reporterzesstra Assigned Tozesstra  
PrioritynormalSeverityfeatureReproducibilityN/A
Status resolvedResolutionfixed 
Product Version3.3 
Fixed in Version3.3.718 
Summary0000519: present_clone(): deep-inventory search and search for <object> no. x
DescriptionRecently I discussed with Honey@Silberland the efun present_clone() and we both missed one or two features for a better replacement of present().
The suggestion it to give the efun additional optional arguments and enable it to
a) perform a deep-inventory search in the given environment
b) enable it to find object no. x, e.g. present_clone("/obj/bottle", this_player(), 4) for finding the forth bottle in the inventory of TP.
TagsNo tags attached.

Relationships

duplicate of 0000255 resolvedzesstra present_clone() should allow 'id <n>' form 

Activities

Gnomi

2008-07-01 05:23

manager   ~0000642

Searching for a n-th object like the present efun is a good idea.

Regarding a) we have to decide how far we want to extend present(_clone). In UNItopia we have one big simul-efun (http://www.unitopia.de/doc/efun/a-m/cond_deep_present.html) that does two forms of deep searching and allows to filter the results using a closure, both could be integrated into these efuns. I'm just not sure whether we should bloat present(_clone) or keep them simple.

zesstra

2008-07-08 17:02

administrator   ~0000686

After some consideration I think, we should implement b) and leave a deep search to the mudlib to keep the interface simpler. (Or use a separate deep_present_clone().) Three optional arguments don't do any good for keeping it clear.
I suggest to match the interface of present_clone() with that of present(), with the exception that something like "bottle 42" is not accepted but has to be given in form of present_clone("bottle", 42):
        object present(string str)
        object present(string str, int n)
        object present(string str, object env)
        object present(string str, int n, object env)

        object present(object ob)
        object present(object ob, object env)

Gnomi

2008-07-09 02:48

manager   ~0000695

Agreed.

zesstra

2008-10-08 17:18

administrator   ~0000805

I prepared a patch for present_clone() which introduces an optional third argument to specify to search for the <n>th object in the given environment. The second argument remains optional as well and defaults to this_object().
The interface would be:
object present_clone(string str [, object env, [int n]])
object present_clone(object obj [, object env, [int n]])

n as second argument would be nicer because it would be the same interface as present() has, but I guess to much code would have to be changed. Therefore I am suggesting to use it as third argument.

Gnomi

2008-10-08 17:29

manager   ~0000806

What kind of code has to be changed? A number as a second argument is now illegal, so there should be no problem with current LPC code.

zesstra

2008-10-08 18:04

administrator   ~0000807

Ok, you are right, if we accept
object present_clone(string|object, object|int|void, object|void);
we may sort it out in the efun.
I had
object present_clone(string|object, int|void, object|void);
in mind which breaks the current interface.

Personally I don't like it very much if arguments can have several different semantics, because it 'dilutes' the interface and makes it somewhat arbitrary, therefore I did not think of this. It complicates argument checks as well and I actually perceive the function as more complex zu remember. But there may be other optinions about this.

zesstra

2008-12-26 12:04

administrator   ~0000830

Ok, call for last comments please. ;-) As I said, I don't like it very much to give arguments more meanings than necessary and personally I prefer
    object present_clone(string|object, int|void, object|void)

But if people are wishing otherwise, I can live with a different interface as well.

Gnomi

2008-12-26 17:44

manager   ~0000831

object present_clone(string|object, int|void, object|void) breaks the current interface, so I don't like that. I'd like to have present_clone() similar to present(). It wouldn't be intuitive if present("id", where) works and present_clone("id", where) doesn't, or if the same arguments got another order in present_clone() than in present(). It is not nice to implement in the driver but I think a plausible interface towards LPC is more important.

And remembering the function as present_clone(string str [, int n] [, object where]) is not hard. And it's not unprecedented that arguments in the middle can be optional (e.g. function_exists, input_to).

zesstra

2008-12-29 05:40

administrator   ~0000833

Gna. My mistake.
I actually meant object present_clone(string|object, object|void, int|void), which does not break the current interface.
You are advocating for
  object present_clone(string|object, object|int|void, int|void)
then?
My critique on this is just that the semantic of the second argument is less defined and the driver can't detect some programming mistakes during the syntax checks any more. And unless I am still not really awake, something like present_clone("bla",ob) can be nasty, if <ob> is destructed and there actually is a different "bla" in this_object().

fufu

2008-12-29 06:47

manager   ~0000836

We can fix the present_clone("foo", 0) ambiguity in two ways:

- present_clone(string|object [, int n] [, object env]), where n is not allowed to be 0, or
- present_clone(string|object [, object env [, int n]]), where env can be 0.

I like the first one slightly better because it mimics present().

(I'd like to forbid passing 0 for n in present as well, but it's far too late for that.)

Gnomi

2008-12-29 10:10

manager   ~0000838

I'm in favor of disallowing 0 as <n> or <env>. Searching the 0th element doesn't make any sense and a destructed object is usually not accepted by any efun.

zesstra

2008-12-29 13:08

administrator   ~0000839

I favor the second possibility (mainly because of the better type-safety (at compile-time) and more stringend interface), but you two are the majority.
But: Searching for the 0th element can make perfect sense (like when indexing arrays), it is just a convention where to start counting. Starting with 1 and accepting 0 as alias to 1 as present() does is of course not very reasonable.

BTW: there are some efuns which accept destructed objects or 0 (e.g. living(), clonep(), object_name(), program_name() and of course present()). Should we think about changing them?

2008-12-31 08:28

 

present_clone.diff (5,749 bytes)   
Index: func_spec
===================================================================
--- func_spec	(Revision 2457)
+++ func_spec	(Arbeitskopie)
@@ -516,7 +516,7 @@
 mixed  *object_info(object, int, void|int);
 string  object_name(null|object default: F_THIS_OBJECT);
 int     object_time(object default: F_THIS_OBJECT);
-object  present_clone(object|string, object default: F_THIS_OBJECT); /* PRELIMINARY */
+object  present_clone(object|string, object|int|void, int|void);
 string  program_name(null|object default: F_THIS_OBJECT);
 int     program_time(object default: F_THIS_OBJECT);
 void    replace_program(void|string);
Index: efuns.h
===================================================================
--- efuns.h	(Revision 2457)
+++ efuns.h	(Arbeitskopie)
@@ -76,7 +76,7 @@
 extern svalue_t *f_blueprint (svalue_t *sp);
 extern svalue_t *v_clones (svalue_t *sp, int num_args);
 extern svalue_t *v_object_info (svalue_t *sp, int num_args);
-extern svalue_t *f_present_clone (svalue_t *sp);
+extern svalue_t *v_present_clone (svalue_t *sp, int num_arg);
 extern svalue_t *f_to_object(svalue_t *sp);
 extern svalue_t *f_set_is_wizard(svalue_t *sp);  /* optional */
 extern svalue_t *tell_room(svalue_t *sp);
Index: efuns.c
===================================================================
--- efuns.c	(Revision 2457)
+++ efuns.c	(Arbeitskopie)
@@ -4473,15 +4473,15 @@
 
 /*-------------------------------------------------------------------------*/
 svalue_t *
-f_present_clone (svalue_t *sp)
+v_present_clone (svalue_t *sp, int num_arg)
 
 /* EFUN present_clone()
  *
- *    object present_clone(string str, object env)
- *    object present_clone(object obj, object env)
+ *    object present_clone(string str [, object env] [, [int n])
+ *    object present_clone(object obj [, object env] [, [int n])
  *
- * Search in the inventory of <env> for the first object with the
- * same blueprint as object <obj>, resp. for the first object with
+ * Search in the inventory of <env> for the <n>th object with the
+ * same blueprint as object <obj>, resp. for the <n>th object with
  * the loadname <str>, and return that object.
  *
  * If not found, 0 is returned.
@@ -4490,10 +4490,48 @@
 {
     string_t * name; /* the shared loadname to look for */
     object_t *obj;   /* the object under scrutiny */
+    object_t *env;   /* the environment to search in */
+    int       count; /* the <count> object is searched */
 
-    /* Test and get the arguments from the stack */
-    if (sp[-1].type == T_STRING)
+    /* Get the arguments */
+    svalue_t *arg = sp - num_arg + 1;   // first argument
+    env = current_object;  // default
+    count = -1;
+    if (num_arg == 3)
     {
+        // if we got 3 args, the third must be a number.
+        count = arg[2].u.number;
+        free_svalue(sp--);
+        num_arg--;
+    }
+    if (num_arg == 2)
+    {
+        // the second arg may be an object or a number
+        if (arg[1].type == T_NUMBER)
+        {
+            // But it must not be 0 (which is probably a destructed object)
+            // and we don't accept two numbers (as second and third arg)
+            if (arg[1].u.number == 0 || count != -1)
+            {
+                vefun_arg_error(2, T_OBJECT, T_NUMBER, sp);
+                return sp; /* NOTREACHED */
+            }
+            count = arg[1].u.number;
+        }
+        else if (arg[1].type == T_OBJECT)
+        {
+            env = arg[1].u.ob;
+        }
+        free_svalue(sp--);
+        num_arg--;
+    }
+    /* no number given? search for the first object */
+    if (count == -1)
+        count = 1;
+
+    /* Get the name/object to search for */
+    if (arg->type == T_STRING)
+    {
         size_t len;
         long i;
         char * end;
@@ -4501,7 +4539,7 @@
         char * name0;  /* Intermediate name */
         char * tmpbuf; /* intermediate buffer for stripping any #xxxx */
         
-        name0 = get_txt(sp[-1].u.str);
+        name0 = get_txt(arg->u.str);
         tmpbuf = NULL;
         
         /* Normalize the given string and check if it is
@@ -4511,7 +4549,7 @@
 
         /* First, slash of a trailing '#<num>' */
 
-        len = mstrsize((sp-1)->u.str);
+        len = mstrsize(arg->u.str);
         i = (long)len;
         end = name0 + len;
 
@@ -4531,7 +4569,7 @@
                         errorf("Out of memory (%ld bytes) for temporary "
                                "buffer in present_clone().\n", i+1);
                     }
-                    strncpy(tmpbuf, get_txt(sp[-1].u.str), (size_t)i);
+                    strncpy(tmpbuf, get_txt(arg->u.str), (size_t)i);
                     name0[i] = '\0';
                 }
 
@@ -4559,26 +4597,27 @@
             tmpbuf = name0 = NULL;
         }
     }
-    else if (sp[-1].type == T_OBJECT)
+    else if (arg->type == T_OBJECT)
     {
-        name = sp[-1].u.ob->load_name;
+        name = arg->u.ob->load_name;
     }
     else
-        efun_gen_arg_error(1, sp[-1].type, sp);
+        vefun_exp_arg_error(1, TF_STRING|TF_OBJECT, arg->type, sp);
 
     obj = NULL;
     if (name)
     {
         /* We have a name, now look for the object */
-        for (obj = sp->u.ob->contains; obj != NULL; obj = obj->next_inv)
+        for (obj = env->contains; obj != NULL; obj = obj->next_inv)
         {
-            if (!(obj->flags & O_DESTRUCTED) && name == obj->load_name)
+            if (!(obj->flags & O_DESTRUCTED) && name == obj->load_name
+                && --count <= 0)
                 break;
         }
     }
 
-    /* Assign the result */
-    sp = pop_n_elems(2, sp) + 1;
+    /* Free first argument and assign the result */
+    free_svalue(sp);
     if (obj != NULL)
         put_ref_object(sp, obj, "present_clone");
     else
present_clone.diff (5,749 bytes)   

zesstra

2008-12-31 08:40

administrator   ~0000844

I applied a patch which implements the
  object present_clone(string|object, object|int|void, int|void)
interface. Preliminary testing looks good, but I have to go now and will test a little bit more while I am away.

zesstra

2009-01-03 12:04

administrator   ~0000845

Slightly improved version of the patch (stricter checks for the count argument) plus documentation changes commited as r2461.

Issue History

Date Modified Username Field Change
2007-09-24 09:41 zesstra New Issue
2008-06-30 04:58 zesstra Status new => assigned
2008-06-30 04:58 zesstra Assigned To => zesstra
2008-07-01 05:23 Gnomi Note Added: 0000642
2008-07-08 17:02 zesstra Note Added: 0000686
2008-07-09 02:48 Gnomi Note Added: 0000695
2008-07-17 07:36 zesstra Relationship added duplicate of 0000255
2008-10-08 17:18 zesstra Note Added: 0000805
2008-10-08 17:19 zesstra File Added: present_clone.diff
2008-10-08 17:29 Gnomi Note Added: 0000806
2008-10-08 18:04 zesstra Note Added: 0000807
2008-12-26 12:04 zesstra Note Added: 0000830
2008-12-26 17:44 Gnomi Note Added: 0000831
2008-12-29 05:40 zesstra Note Added: 0000833
2008-12-29 06:47 fufu Note Added: 0000836
2008-12-29 10:10 Gnomi Note Added: 0000838
2008-12-29 13:08 zesstra Note Added: 0000839
2008-12-30 17:48 zesstra File Deleted: present_clone.diff
2008-12-31 08:28 zesstra File Added: present_clone.diff
2008-12-31 08:40 zesstra Note Added: 0000844
2009-01-03 12:04 zesstra Note Added: 0000845
2009-01-03 12:04 zesstra Status assigned => resolved
2009-01-03 12:04 zesstra Fixed in Version => 3.3.718
2009-01-03 12:04 zesstra Resolution open => fixed