View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000519 | LDMud 3.3 | Efuns | public | 2007-09-24 07:41 | 2009-01-03 11:04 |
Reporter | zesstra | Assigned To | zesstra | ||
Priority | normal | Severity | feature | Reproducibility | N/A |
Status | resolved | Resolution | fixed | ||
Product Version | 3.3 | ||||
Fixed in Version | 3.3.718 | ||||
Summary | 0000519: present_clone(): deep-inventory search and search for <object> no. x | ||||
Description | Recently 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. | ||||
Tags | No tags attached. | ||||
Attached Files | 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 | ||||
|
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. |
|
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) |
|
Agreed. |
|
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. |
|
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. |
|
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. |
|
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. |
|
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). |
|
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(). |
|
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.) |
|
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. |
|
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? |
|
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. |
|
Slightly improved version of the patch (stricter checks for the count argument) plus documentation changes commited as r2461. |
Date Modified | Username | Field | Change |
---|---|---|---|
2007-09-24 07:41 | zesstra | New Issue | |
2008-06-30 02:58 | zesstra | Status | new => assigned |
2008-06-30 02:58 | zesstra | Assigned To | => zesstra |
2008-07-01 03:23 | Gnomi | Note Added: 0000642 | |
2008-07-08 15:02 | zesstra | Note Added: 0000686 | |
2008-07-09 00:48 | Gnomi | Note Added: 0000695 | |
2008-07-17 05:36 | zesstra | Relationship added | duplicate of 0000255 |
2008-10-08 15:18 | zesstra | Note Added: 0000805 | |
2008-10-08 15:19 | zesstra | File Added: present_clone.diff | |
2008-10-08 15:29 | Gnomi | Note Added: 0000806 | |
2008-10-08 16:04 | zesstra | Note Added: 0000807 | |
2008-12-26 11:04 | zesstra | Note Added: 0000830 | |
2008-12-26 16:44 | Gnomi | Note Added: 0000831 | |
2008-12-29 04:40 | zesstra | Note Added: 0000833 | |
2008-12-29 05:47 | fufu | Note Added: 0000836 | |
2008-12-29 09:10 | Gnomi | Note Added: 0000838 | |
2008-12-29 12:08 | zesstra | Note Added: 0000839 | |
2008-12-30 16:48 | zesstra | File Deleted: present_clone.diff | |
2008-12-31 07:28 | zesstra | File Added: present_clone.diff | |
2008-12-31 07:40 | zesstra | Note Added: 0000844 | |
2009-01-03 11:04 | zesstra | Note Added: 0000845 | |
2009-01-03 11:04 | zesstra | Status | assigned => resolved |
2009-01-03 11:04 | zesstra | Fixed in Version | => 3.3.718 |
2009-01-03 11:04 | zesstra | Resolution | open => fixed |