On Thu, 2009-08-13 at 15:34 +0800, Bo Yang wrote:
> Hi jmb,
>
> Thanks for the review. I have fixed most of the problems, and
> following are my new comments.
>
> >> +
> >> + if (strncmp(data, "AltGraph", len) == 0) {
> >> + *state = evt->alt_graph;
> >> + }
> >
> > The point of interning strings is that you don't need to call strncmp to
> > compare them.
>
> There are 8 strings together, so I think this don't need a lwc style
> compare and it is efficient if I add the len compare before the real
> string compare.
Ok. I don't imagine this is hugely speed-critical, anyway. It can always
be changed later.
> >> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> >> +++ src/events/event_target.c 2009-08-12 15:12:50.000000000 +0100
> >> +
> >> +/* Hash key/value functions */
> >> +static void *_key(void *key, void *key_pw, dom_alloc alloc, void *pw,
> >> + bool clone);
> >> +static void *_value(void *value, void *value_pw, dom_alloc alloc,
> >> + void *pw, bool clone);
> >> +static unsigned int _dom_element_hash_lwcstring(void *key);
> >
> > This needs renaming. More importantly, can it be put somewhere central
> > -- in src/utils, perhaps? I'd like to avoid duplicating this kind of
> > functionality.
>
> I have move the _dom_element_hash_lwcstring to utils/hash_table.c. And
> the _key and _value functions must be here and each hash table needs
> itself's _key and _value, so they can not be shared. The code insides
> these two functions are different between different hash tables. :)
Yeah. My comment applied only to the hash function. The others are fine.
> >> --- src/utils/namespace.c (revision 9213)
> >> +++ src/utils/namespace.c (working copy)
> >> @@ -151,13 +151,6 @@
> >>
> >> if (colon == (uint32_t) -1) {
> >> /* No prefix */
> >> - /* If namespace URI is for xmlns, ensure qname == "xmlns" */
> >> - if (namespace != NULL &&
> >> - dom_string_cmp(namespace,
> >> - dom_namespaces[DOM_NAMESPACE_XMLNS]) == 0 &&
> >> - dom_string_cmp(qname, xmlns) != 0) {
> >> - return DOM_NAMESPACE_ERR;
> >> - }
> >> /* If qname == "xmlns", ensure namespace URI is for xmlns */
> >> if (namespace != NULL &&
> >> dom_string_cmp(qname, xmlns) == 0 &&
> >
> > It turns out that I got the merge wrong here. Ignore this change.
>
> These are some changes I commit to the turnk/dom yestoday, I don't
> understand why they are here.
Because the diff is produced as a merge against trunk.
> >> Index: src/core/string.c
> >> ===================================================================
> >> --- src/core/string.c (revision 9213)
> >> +++ src/core/string.c (working copy)
> >> @@ -56,7 +56,8 @@
> >> */
> >> void dom_string_ref(struct dom_string *str)
> >> {
> >> - str->refcnt++;
> >> + if (str != NULL)
> >> + str->refcnt++;
> >> }
> >
> > Surely it's a bug if str is ever NULL here?
>
> For the NULL pointer problems, both for string and node. I think it is
> better to test a whether a Node/String is NULL in the ref/unref
> function, it should not be treated as an error when a NULL passed into
> these two functions.
> If we don't test the NULL inside these functions, we should write many
> code like:
>
> if (p != NULL)
> dom_node_(un)ref(p);
>
> If something can't be NULL, we should test it in the right place
> instead of making a (un)ref function crashed.
Perhaps. I still think it's a bug to be passing NULL into these
functions :)
> >> @@ -1007,10 +1016,6 @@
> >> _dom_node_detach(new_child);
> >> }
> >>
> >> - /* When a Node is attached, it should be removed from the pending
> >> - * list */
> >> - dom_node_remove_pending(new_child);
> >> -
> >
> > Where does it get removed from the pending list now?
>
> This is a change from the trunk/dom, I don't understand why it is in
> this review. For the problem, when some node is _dom_node_detach, it
> is put into the pending_list, so we should remove it from the
> pending_list before we attach it.
As above.
J.
