Re: [PATCH 3/3] reader: Use xmlFirstElementChild/xmlNextElementSibling to iterate over children elements

[Giuliano Procida <gprocida@google.com>]

Hi there.

On Mon, 3 May 2021 at 16:18, Dodji Seketeli <dodji@seketeli.org> wrote:

> Hello,
>
> [...]
>
> Dodji Seketeli <dodji@seketeli.org> a écrit:
>
> > Hello Giuliano,
> >
> > Giuliano Procida <gprocida@google.com> a écrit:
> >
> >
> >> I had a quick look and there's (at least) one place where there still a
> >> direct access to ->children. The code works because there are separate
> >> tests to not process wrong nodes, but it would be neater to use the
> first
> >> element child function uniformly (as well).
> >
> > You are right.
> >
> > Below is an update of that patch that hopefully addresses your comment.
> >
> > Thanks.
> >
> > From 0a6c322f6ec8be354122c6b4cdefe6143eb568ed Mon Sep 17 00:00:00 2001
> > From: Dodji Seketeli <dodji@redhat.com>
> > Date: Mon, 19 Apr 2021 13:50:33 +0200
> > Subject: [PATCH 3/3] reader: Use
> xmlFirstElementChild/xmlNextElementSibling to
> >  iterate over children elements
> >
> > Use xmlFirstElementChild/xmlNextElementSibling to iterate over element
> > children nodes rather than doing it by hand in the various for loops.
> >
> >       * src/abg-reader.cc (walk_xml_node_to_map_type_ids)
> >       (read_translation_unit, read_translation_unit_from_input)
> >       (read_symbol_db_from_input, build_needed)
> >       (read_elf_needed_from_input, read_corpus_group_from_input)
> >       (build_namespace_decl, build_elf_symbol_db, build_function_decl)
> >       (build_function_type, build_array_type_def, build_enum_type_decl)
> >       (build_class_decl, build_union_decl, build_function_tdecl)
> >       (build_class_tdecl, build_type_composition)
> >       (build_template_tparameter): Use
> >       xmlFirstElementChild/xmlNextElementSibling rather than poking at
> >       xmlNode::children and looping over xmlNode::next by hand.
> >
> > Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>
> I have just applied these 3 patches to master.  I forgot to do this
> after I posted the patches and saw no follow-up here.
>
>
I was going to compare with my version as a last check. Your version has
now landed in public master. List of the differences:

walk_xml_node_to_map_type_ids - extra null test and node type test -
redundant as all callers pass in an XML element node pointer
read_translation_unit_from_input - the Tree API branch that gets the next
abi-instr has a for loop, but it can be coded without this; I think the
final test of ctxt.get_corpus_node() is redundant
build_needed - extra null test - redundant
read_elf_needed_from_input - the Tree API branch that gets the next
abi-instr has a for loop, can be done without a loop
read_corpus_from_input - no need to test node->children, just do the update
unconditionally; I was also able to get rid of the final else clause
altogether
read_corpus_group_from_input - while loop - this can be expressed as a for
loop over the child elements
build_class_decl and build_union_decl - the code now processes the child
elements of declaration-only types which it did not do before
(and master omits some unnecessary { } block braces I had a in a few places)

To summarise:

I must have missed that last remaining use of node->children in my previous
review, sorry.
You've changed the behaviour of build_class_decl and build_union_decl, but
it probably makes no difference. See
https://sourceware.org/bugzilla/show_bug.cgi?id=26591#c7.
Most of the remaining differences relate to my making the parsing use fewer
conditionals, simpler loops etc.

Regards,
Giuliano.


> Thanks for the review!
>
> [...]
>
> Cheers,
>
> --
>                 Dodji
>