Hi Patrick,
Thanks for your comments. See below.
El dom, 29-01-2006 a las 20:39 +0100, Patrick Bernaud escribió:
[snip]
> A few comments on your patch that looks good to me overall:
>
> - are g_get_object/toplevel_from_object_smob() really required?
> Firstly, the smob structure is public. Secondly since they are
> often used one after the other it duplicates the assertion. And
> finally if an error occur, the error message will display
> "get_object/toplevel_from_object_smob" as the source and these
> functions are unknown from user. I would then suggest putting the
> assertion in g_add_attrib() and let it get fields of the structure
> without these accessors.
They are not required. I added them because of the assertion:
SCM_ASSERT ( SCM_NIMP(object_smob) &&
(SCM_CAR(object_smob) == object_smob_tag),
object_smob, SCM_ARG1, "get_toplevel_from_object_smob");
It uses attrib_smob_tag, which is only defined inside g_smob.nw, as
attrib_smob_tag. I thought that there is no benefit in exposing the
object_smob_tag to the global variables space, so I added those two
functions. I have several options:
- Keep the object_smob_tag inside g_smob.nw, and write a function to
check the object_smob type inside g_smob.nw:
gboolean is_object_smob(SCM smob);
This way, I can keep the SCM_ASSERT in the calling function
(g_add_attrib), so there is no need to expose the object_smob_tag.
However, the calling function should know how the object smob struct is
declared and do all the data extraction.
- Write a gboolean g_get_data_from_object_smob(SCM object, TOPLEVEL
**wcurrent, OBJECT **object). It will make only one type checking,
return the toplevel and object pointers, and the return value could be
true if all is succesful, or false if there was a type checking error.
The SCM_ASSERT could be kept in the calling function.
- Make the object_smob_tag global and do all the job in g_add_attrib,
as you suggest.
Personally, I prefer to encapsulate the functions to access a given data
set (that's why I added the g_get_object/toplevel_from_object_smob), and
just call that functions instead of dealing with the data directly
(therefore I like option 2). I mean: if I want to get the data from the
object smob, I'd like that I don't need to know how the object smob
struct is declared.
What do you think?
> - please remove Ales old comment in g_print_object_smob() as
> evaluation is indeed stopped after first failure.
Fixed.
> - in system-gschemrc, I would recommend the definition of a variable
> for the details of the attributes and modify
> add-default-pin-attributes accordingly:
>
> (define default-pin-attributes
> '(("pintype" "unknown" #f ())
> ("pinlabel" "unknown" #t (value))
> ...))
> (define (add-default-pin-attributes object)
> (for-each
> (lambda (a)
> (apply add-attribute-to-object object a)) default-pin-attributes))
>
> The user can change default-pin-attributes without rewritting its
> own hook function.
Agreed. Changed accordingly.
> - your patch is adding a useless blank line in hooks_and_scheme.txt
> between 'Inparameter' and 'Outparameter' of
> set-attribute-value!.
Fixed.
Thank you very much for your help. Best regards,
Carlos
