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

  Hello,

Use xmlFirstElementChild/xmlNextElementSibling to iterate over element
children nodes rather than doing it by hand in the various for loops.

	* src/abg-reader.cc (read_translation_unit)
	(read_translation_unit_from_input, read_symbol_db_from_input)
	(build_needed, read_elf_needed_from_input, build_namespace_decl)
	(build_function_decl, build_function_type, build_array_type_def)
	(build_enum_type_decl, build_class_decl, build_union_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>
---
 src/abg-reader.cc | 187 +++++++++++++++++++---------------------------
 1 file changed, 78 insertions(+), 109 deletions(-)
  

Hi.

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).

Regards,
Giuliano.

On Mon, 19 Apr 2021, 15:03 Dodji Seketeli, <dodji@seketeli.org> wrote:

> Hello,
>
> Use xmlFirstElementChild/xmlNextElementSibling to iterate over element
> children nodes rather than doing it by hand in the various for loops.
>
>         * src/abg-reader.cc (read_translation_unit)
>         (read_translation_unit_from_input, read_symbol_db_from_input)
>         (build_needed, read_elf_needed_from_input, build_namespace_decl)
>         (build_function_decl, build_function_type, build_array_type_def)
>         (build_enum_type_decl, build_class_decl, build_union_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>
> ---
>  src/abg-reader.cc | 187 +++++++++++++++++++---------------------------
>  1 file changed, 78 insertions(+), 109 deletions(-)
>
> diff --git a/src/abg-reader.cc b/src/abg-reader.cc
> index f5ed87b2..c214b755 100644
> --- a/src/abg-reader.cc
> +++ b/src/abg-reader.cc
> @@ -1396,12 +1396,8 @@ read_translation_unit(read_context& ctxt,
> translation_unit& tu, xmlNodePtr node)
>        || !ctxt.get_corpus())
>      walk_xml_node_to_map_type_ids(ctxt, node);
>
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> -    {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -      handle_element_node(ctxt, n, /*add_decl_to_scope=*/true);
> -    }
> +  for (xmlNodePtr n = node->children; n; n = xmlNextElementSibling(n))
> +    handle_element_node(ctxt, n, /*add_decl_to_scope=*/true);
>
>    ctxt.pop_scope_or_abort(tu.get_global_scope());
>
> @@ -1495,11 +1491,10 @@ read_translation_unit_from_input(read_context&
> ctxt)
>    else
>      {
>        node = 0;
> -      for (xmlNodePtr n = ctxt.get_corpus_node(); n; n = n->next)
> +      for (xmlNodePtr n = ctxt.get_corpus_node();
> +          n;
> +          n = xmlNextElementSibling(n))
>         {
> -         if (!n
> -             || n->type != XML_ELEMENT_NODE)
> -           continue;
>           if (!xmlStrEqual(n->name, BAD_CAST("abi-instr")))
>             return nil;
>           node = n;
> @@ -1595,11 +1590,8 @@ read_symbol_db_from_input(read_context&
>  ctxt,
>         xmlTextReaderNext(reader.get());
>        }
>    else
> -    for (xmlNodePtr n = ctxt.get_corpus_node(); n; n = n->next)
> +    for (xmlNodePtr n = ctxt.get_corpus_node(); n; n =
> xmlNextElementSibling(n))
>        {
> -       if (!n || n->type != XML_ELEMENT_NODE)
> -         continue;
> -
>         bool has_fn_syms = false, has_var_syms = false;
>         if (xmlStrEqual(n->name, BAD_CAST("elf-function-symbols")))
>           has_fn_syms = true;
> @@ -1647,10 +1639,9 @@ build_needed(xmlNode* node, vector<string>& needed)
>    if (!node || !xmlStrEqual(node->name,BAD_CAST("elf-needed")))
>      return false;
>
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> +  for (xmlNodePtr n = node->children; n; n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE
> -         || !xmlStrEqual(n->name, BAD_CAST("dependency")))
> +      if (!xmlStrEqual(n->name, BAD_CAST("dependency")))
>         continue;
>
>        string name;
> @@ -1703,10 +1694,10 @@ read_elf_needed_from_input(read_context&
> ctxt,
>      }
>    else
>      {
> -      for (xmlNodePtr n = ctxt.get_corpus_node(); n; n = n->next)
> +      for (xmlNodePtr n = ctxt.get_corpus_node();
> +          n;
> +          n = xmlNextElementSibling(n))
>         {
> -         if (!n || n->type != XML_ELEMENT_NODE)
> -           continue;
>           if (!xmlStrEqual(n->name, BAD_CAST("elf-needed")))
>             return false;
>           node = n;
> @@ -2746,12 +2737,10 @@ build_namespace_decl(read_context&      ctxt,
>    ctxt.push_decl_to_current_scope(decl, add_to_current_scope);
>    ctxt.map_xml_node_to_decl(node, decl);
>
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> -    {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -      handle_element_node(ctxt, n, /*add_to_current_scope=*/true);
> -    }
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
> +    handle_element_node(ctxt, n, /*add_to_current_scope=*/true);
>
>    ctxt.pop_scope_or_abort(decl);
>
> @@ -3104,12 +3093,11 @@ build_function_decl(read_context&       ctxt,
>    std::vector<function_decl::parameter_sptr> parms;
>    type_base_sptr return_type = env->get_void_type();
>
> -  for (xmlNodePtr n = node->children; n ; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n ;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
> -      else if (xmlStrEqual(n->name, BAD_CAST("parameter")))
> +      if (xmlStrEqual(n->name, BAD_CAST("parameter")))
>         {
>           if (function_decl::parameter_sptr p =
>               build_function_parameter(ctxt, n))
> @@ -3797,12 +3785,11 @@ build_function_type(read_context&       ctxt,
>    ctxt.get_translation_unit()->bind_function_type_life_time(fn_type);
>    ctxt.key_type_decl(fn_type, id);
>
> -  for (xmlNodePtr n = node->children; n ; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
> -      else if (xmlStrEqual(n->name, BAD_CAST("parameter")))
> +      if (xmlStrEqual(n->name, BAD_CAST("parameter")))
>         {
>           if (function_decl::parameter_sptr p =
>               build_function_parameter(ctxt, n))
> @@ -4033,12 +4020,11 @@ build_array_type_def(read_context&      ctxt,
>    read_location(ctxt, node, loc);
>    array_type_def::subranges_type subranges;
>
> -  for (xmlNodePtr n = node->children; n ; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
> -      else if (xmlStrEqual(n->name, BAD_CAST("subrange")))
> +      if (xmlStrEqual(n->name, BAD_CAST("subrange")))
>         {
>           if (array_type_def::subrange_sptr s =
>               build_subrange_type(ctxt, n))
> @@ -4191,11 +4177,10 @@ build_enum_type_decl(read_context&      ctxt,
>
>    string base_type_id;
>    enum_type_decl::enumerators enums;
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
>        if (xmlStrEqual(n->name, BAD_CAST("underlying-type")))
>         {
>           xml_char_sptr a = xml::build_sptr(xmlGetProp(n,
> BAD_CAST("type-id")));
> @@ -4562,11 +4547,10 @@ build_class_decl(read_context&          ctxt,
>        decl->set_naming_typedef(naming_typedef);
>      }
>
> -  for (xmlNodePtr n = node->children; !is_decl_only && n; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       !is_decl_only && n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
>        if (xmlStrEqual(n->name, BAD_CAST("base-class")))
>         {
>           access_specifier access =
> @@ -4613,11 +4597,10 @@ build_class_decl(read_context&          ctxt,
>
>           ctxt.map_xml_node_to_decl(n, decl);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (type_base_sptr t =
>                   build_type(ctxt, p, /*add_to_current_scope=*/true))
>                 {
> @@ -4651,11 +4634,10 @@ build_class_decl(read_context&          ctxt,
>           bool is_static = false;
>           read_static(n, is_static);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (var_decl_sptr v =
>                   build_var_decl(ctxt, p, /*add_to_cur_scope=*/false))
>                 {
> @@ -4709,11 +4691,10 @@ build_class_decl(read_context&          ctxt,
>           bool is_ctor = false, is_dtor = false, is_const = false;
>           read_cdtor_const(n, is_ctor, is_dtor, is_const);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (function_decl_sptr f =
>                   build_function_decl_if_not_suppressed(ctxt, p, decl,
>
> /*add_to_cur_sc=*/true))
> @@ -4748,11 +4729,10 @@ build_class_decl(read_context&          ctxt,
>           bool is_ctor = false, is_dtor = false, is_const = false;
>           read_cdtor_const(n, is_ctor, is_dtor, is_const);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (shared_ptr<function_tdecl> f =
>                   build_function_tdecl(ctxt, p,
>                                        /*add_to_current_scope=*/true))
> @@ -4962,11 +4942,10 @@ build_union_decl(read_context& ctxt,
>    ctxt.map_xml_node_to_decl(node, decl);
>    ctxt.key_type_decl(decl, id);
>
> -  for (xmlNodePtr n = node->children; !is_decl_only && n; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       !is_decl_only && n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
>        if (xmlStrEqual(n->name, BAD_CAST("member-type")))
>         {
>           access_specifier access = private_access;
> @@ -4974,11 +4953,10 @@ build_union_decl(read_context& ctxt,
>
>           ctxt.map_xml_node_to_decl(n, decl);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (type_base_sptr t =
>                   build_type(ctxt, p, /*add_to_current_scope=*/true))
>                 {
> @@ -5006,11 +4984,10 @@ build_union_decl(read_context& ctxt,
>           bool is_static = false;
>           read_static(n, is_static);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (var_decl_sptr v =
>                   build_var_decl(ctxt, p, /*add_to_cur_scope=*/false))
>                 {
> @@ -5048,11 +5025,10 @@ build_union_decl(read_context& ctxt,
>           bool is_ctor = false, is_dtor = false, is_const = false;
>           read_cdtor_const(n, is_ctor, is_dtor, is_const);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (function_decl_sptr f =
>                   build_function_decl_if_not_suppressed(ctxt, p, decl,
>
> /*add_to_cur_sc=*/true))
> @@ -5081,11 +5057,10 @@ build_union_decl(read_context& ctxt,
>           bool is_ctor = false, is_dtor = false, is_const = false;
>           read_cdtor_const(n, is_ctor, is_dtor, is_const);
>
> -         for (xmlNodePtr p = n->children; p; p = p->next)
> +         for (xmlNodePtr p = xmlFirstElementChild(n);
> +              p;
> +              p = xmlNextElementSibling(p))
>             {
> -             if (p->type != XML_ELEMENT_NODE)
> -               continue;
> -
>               if (function_tdecl_sptr f =
>                   build_function_tdecl(ctxt, p,
>                                        /*add_to_current_scope=*/true))
> @@ -5160,11 +5135,10 @@ build_function_tdecl(read_context& ctxt,
>    ctxt.push_decl_to_current_scope(fn_tmpl_decl, add_to_current_scope);
>
>    unsigned parm_index = 0;
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
>        if (template_parameter_sptr parm =
>           build_template_parameter(ctxt, n, parm_index, fn_tmpl_decl))
>         {
> @@ -5224,11 +5198,10 @@ build_class_tdecl(read_context& ctxt,
>    ctxt.push_decl_to_current_scope(class_tmpl, add_to_current_scope);
>
>    unsigned parm_index = 0;
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
>        if (template_parameter_sptr parm=
>           build_template_parameter(ctxt, n, parm_index, class_tmpl))
>         {
> @@ -5343,11 +5316,10 @@ build_type_composition(read_context&
> ctxt,
>    ctxt.push_decl_to_current_scope(dynamic_pointer_cast<decl_base>(result),
>                                   /*add_to_current_scope=*/true);
>
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
>      {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
>        if ((composed_type =
>            build_pointer_type_def(ctxt, n,
>                                   /*add_to_current_scope=*/true))
> @@ -5473,18 +5445,15 @@ build_template_tparameter(read_context& ctxt,
>
>    // Go parse template parameters that are children nodes
>    int parm_index = 0;
> -  for (xmlNodePtr n = node->children; n; n = n->next)
> -    {
> -      if (n->type != XML_ELEMENT_NODE)
> -       continue;
> -
> -      if (shared_ptr<template_parameter> p =
> -         build_template_parameter(ctxt, n, parm_index, result))
> -       {
> -         result->add_template_parameter(p);
> -         ++parm_index;
> -       }
> -    }
> +  for (xmlNodePtr n = xmlFirstElementChild(node);
> +       n;
> +       n = xmlNextElementSibling(n))
> +    if (shared_ptr<template_parameter> p =
> +       build_template_parameter(ctxt, n, parm_index, result))
> +      {
> +       result->add_template_parameter(p);
> +       ++parm_index;
> +      }
>
>    if (result)
>      {
> --
> 2.30.0
>
>
>
> --
>                 Dodji
>
  

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>
---
 src/abg-reader.cc | 239 ++++++++++++++++++++--------------------------
 1 file changed, 103 insertions(+), 136 deletions(-)

diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index f5ed87b2..39629314 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -1354,7 +1354,7 @@ walk_xml_node_to_map_type_ids(read_context& ctxt,
       ctxt.map_id_and_node(id, n);
     }
 
-  for (n = n->children; n; n = n->next)
+  for (n = xmlFirstElementChild(n); n; n = xmlNextElementSibling(n))
     walk_xml_node_to_map_type_ids(ctxt, n);
 }
 
@@ -1396,12 +1396,10 @@ read_translation_unit(read_context& ctxt, translation_unit& tu, xmlNodePtr node)
       || !ctxt.get_corpus())
     walk_xml_node_to_map_type_ids(ctxt, node);
 
-  for (xmlNodePtr n = node->children; n; n = n->next)
-    {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-      handle_element_node(ctxt, n, /*add_decl_to_scope=*/true);
-    }
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
+    handle_element_node(ctxt, n, /*add_decl_to_scope=*/true);
 
   ctxt.pop_scope_or_abort(tu.get_global_scope());
 
@@ -1495,11 +1493,10 @@ read_translation_unit_from_input(read_context&	ctxt)
   else
     {
       node = 0;
-      for (xmlNodePtr n = ctxt.get_corpus_node(); n; n = n->next)
+      for (xmlNodePtr n = ctxt.get_corpus_node();
+	   n;
+	   n = xmlNextElementSibling(n))
 	{
-	  if (!n
-	      || n->type != XML_ELEMENT_NODE)
-	    continue;
 	  if (!xmlStrEqual(n->name, BAD_CAST("abi-instr")))
 	    return nil;
 	  node = n;
@@ -1595,11 +1592,8 @@ read_symbol_db_from_input(read_context&		 ctxt,
 	xmlTextReaderNext(reader.get());
       }
   else
-    for (xmlNodePtr n = ctxt.get_corpus_node(); n; n = n->next)
+    for (xmlNodePtr n = ctxt.get_corpus_node(); n; n = xmlNextElementSibling(n))
       {
-	if (!n || n->type != XML_ELEMENT_NODE)
-	  continue;
-
 	bool has_fn_syms = false, has_var_syms = false;
 	if (xmlStrEqual(n->name, BAD_CAST("elf-function-symbols")))
 	  has_fn_syms = true;
@@ -1641,16 +1635,14 @@ read_symbol_db_from_input(read_context&		 ctxt,
 static bool
 build_needed(xmlNode* node, vector<string>& needed)
 {
-  if (!node)
-    return false;
-
   if (!node || !xmlStrEqual(node->name,BAD_CAST("elf-needed")))
     return false;
 
-  for (xmlNodePtr n = node->children; n; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE
-	  || !xmlStrEqual(n->name, BAD_CAST("dependency")))
+      if (!xmlStrEqual(n->name, BAD_CAST("dependency")))
 	continue;
 
       string name;
@@ -1703,10 +1695,10 @@ read_elf_needed_from_input(read_context&	ctxt,
     }
   else
     {
-      for (xmlNodePtr n = ctxt.get_corpus_node(); n; n = n->next)
+      for (xmlNodePtr n = ctxt.get_corpus_node();
+	   n;
+	   n = xmlNextElementSibling(n))
 	{
-	  if (!n || n->type != XML_ELEMENT_NODE)
-	    continue;
 	  if (!xmlStrEqual(n->name, BAD_CAST("elf-needed")))
 	    return false;
 	  node = n;
@@ -2054,7 +2046,6 @@ read_corpus_group_from_input(read_context& ctxt)
   if (!node)
     return nil;
 
-  //node = xml::get_first_element_sibling_if_text(node->children);
   node = xmlFirstElementChild(node);
   ctxt.set_corpus_node(node);
 
@@ -2746,12 +2737,10 @@ build_namespace_decl(read_context&	ctxt,
   ctxt.push_decl_to_current_scope(decl, add_to_current_scope);
   ctxt.map_xml_node_to_decl(node, decl);
 
-  for (xmlNodePtr n = node->children; n; n = n->next)
-    {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-      handle_element_node(ctxt, n, /*add_to_current_scope=*/true);
-    }
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
+    handle_element_node(ctxt, n, /*add_to_current_scope=*/true);
 
   ctxt.pop_scope_or_abort(decl);
 
@@ -2938,14 +2927,14 @@ build_elf_symbol_db(read_context& ctxt,
   xml_node_ptr_elf_symbol_sptr_map_type xml_node_ptr_elf_symbol_map;
 
   elf_symbol_sptr sym;
-  for (xmlNodePtr n = node->children; n; n = n->next)
-    {
-      if ((sym = build_elf_symbol(ctxt, n, /*drop_if_suppress=*/false)))
-	{
-	  id_sym_map[sym->get_id_string()] = sym;
-	  xml_node_ptr_elf_symbol_map[n] = sym;
-	}
-    }
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
+    if ((sym = build_elf_symbol(ctxt, n, /*drop_if_suppress=*/false)))
+      {
+	id_sym_map[sym->get_id_string()] = sym;
+	xml_node_ptr_elf_symbol_map[n] = sym;
+      }
 
   if (id_sym_map.empty())
     return nil;
@@ -3104,12 +3093,11 @@ build_function_decl(read_context&	ctxt,
   std::vector<function_decl::parameter_sptr> parms;
   type_base_sptr return_type = env->get_void_type();
 
-  for (xmlNodePtr n = node->children; n ; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n ;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
-      else if (xmlStrEqual(n->name, BAD_CAST("parameter")))
+      if (xmlStrEqual(n->name, BAD_CAST("parameter")))
 	{
 	  if (function_decl::parameter_sptr p =
 	      build_function_parameter(ctxt, n))
@@ -3797,12 +3785,11 @@ build_function_type(read_context&	ctxt,
   ctxt.get_translation_unit()->bind_function_type_life_time(fn_type);
   ctxt.key_type_decl(fn_type, id);
 
-  for (xmlNodePtr n = node->children; n ; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
-      else if (xmlStrEqual(n->name, BAD_CAST("parameter")))
+      if (xmlStrEqual(n->name, BAD_CAST("parameter")))
 	{
 	  if (function_decl::parameter_sptr p =
 	      build_function_parameter(ctxt, n))
@@ -4033,25 +4020,22 @@ build_array_type_def(read_context&	ctxt,
   read_location(ctxt, node, loc);
   array_type_def::subranges_type subranges;
 
-  for (xmlNodePtr n = node->children; n ; n = n->next)
-    {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
-      else if (xmlStrEqual(n->name, BAD_CAST("subrange")))
-	{
-	  if (array_type_def::subrange_sptr s =
-	      build_subrange_type(ctxt, n))
-	    {
-	      if (add_to_current_scope)
-		{
-		  add_decl_to_scope(s, ctxt.get_cur_scope());
-		  ctxt.maybe_canonicalize_type(s);
-		}
-	      subranges.push_back(s);
-	    }
-	}
-    }
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
+    if (xmlStrEqual(n->name, BAD_CAST("subrange")))
+      {
+	if (array_type_def::subrange_sptr s =
+	    build_subrange_type(ctxt, n))
+	  {
+	    if (add_to_current_scope)
+	      {
+		add_decl_to_scope(s, ctxt.get_cur_scope());
+		ctxt.maybe_canonicalize_type(s);
+	      }
+	    subranges.push_back(s);
+	  }
+      }
 
   array_type_def_sptr ar_type(new array_type_def(type,
 						 subranges,
@@ -4191,11 +4175,10 @@ build_enum_type_decl(read_context&	ctxt,
 
   string base_type_id;
   enum_type_decl::enumerators enums;
-  for (xmlNodePtr n = node->children; n; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
       if (xmlStrEqual(n->name, BAD_CAST("underlying-type")))
 	{
 	  xml_char_sptr a = xml::build_sptr(xmlGetProp(n, BAD_CAST("type-id")));
@@ -4562,11 +4545,10 @@ build_class_decl(read_context&		ctxt,
       decl->set_naming_typedef(naming_typedef);
     }
 
-  for (xmlNodePtr n = node->children; !is_decl_only && n; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       !is_decl_only && n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
       if (xmlStrEqual(n->name, BAD_CAST("base-class")))
 	{
 	  access_specifier access =
@@ -4613,11 +4595,10 @@ build_class_decl(read_context&		ctxt,
 
 	  ctxt.map_xml_node_to_decl(n, decl);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (type_base_sptr t =
 		  build_type(ctxt, p, /*add_to_current_scope=*/true))
 		{
@@ -4651,11 +4632,10 @@ build_class_decl(read_context&		ctxt,
 	  bool is_static = false;
 	  read_static(n, is_static);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (var_decl_sptr v =
 		  build_var_decl(ctxt, p, /*add_to_cur_scope=*/false))
 		{
@@ -4709,11 +4689,10 @@ build_class_decl(read_context&		ctxt,
 	  bool is_ctor = false, is_dtor = false, is_const = false;
 	  read_cdtor_const(n, is_ctor, is_dtor, is_const);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (function_decl_sptr f =
 		  build_function_decl_if_not_suppressed(ctxt, p, decl,
 							/*add_to_cur_sc=*/true))
@@ -4748,11 +4727,10 @@ build_class_decl(read_context&		ctxt,
 	  bool is_ctor = false, is_dtor = false, is_const = false;
 	  read_cdtor_const(n, is_ctor, is_dtor, is_const);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (shared_ptr<function_tdecl> f =
 		  build_function_tdecl(ctxt, p,
 				       /*add_to_current_scope=*/true))
@@ -4962,11 +4940,10 @@ build_union_decl(read_context& ctxt,
   ctxt.map_xml_node_to_decl(node, decl);
   ctxt.key_type_decl(decl, id);
 
-  for (xmlNodePtr n = node->children; !is_decl_only && n; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       !is_decl_only && n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
       if (xmlStrEqual(n->name, BAD_CAST("member-type")))
 	{
 	  access_specifier access = private_access;
@@ -4974,11 +4951,10 @@ build_union_decl(read_context& ctxt,
 
 	  ctxt.map_xml_node_to_decl(n, decl);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (type_base_sptr t =
 		  build_type(ctxt, p, /*add_to_current_scope=*/true))
 		{
@@ -5006,11 +4982,10 @@ build_union_decl(read_context& ctxt,
 	  bool is_static = false;
 	  read_static(n, is_static);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (var_decl_sptr v =
 		  build_var_decl(ctxt, p, /*add_to_cur_scope=*/false))
 		{
@@ -5048,11 +5023,10 @@ build_union_decl(read_context& ctxt,
 	  bool is_ctor = false, is_dtor = false, is_const = false;
 	  read_cdtor_const(n, is_ctor, is_dtor, is_const);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (function_decl_sptr f =
 		  build_function_decl_if_not_suppressed(ctxt, p, decl,
 							/*add_to_cur_sc=*/true))
@@ -5081,11 +5055,10 @@ build_union_decl(read_context& ctxt,
 	  bool is_ctor = false, is_dtor = false, is_const = false;
 	  read_cdtor_const(n, is_ctor, is_dtor, is_const);
 
-	  for (xmlNodePtr p = n->children; p; p = p->next)
+	  for (xmlNodePtr p = xmlFirstElementChild(n);
+	       p;
+	       p = xmlNextElementSibling(p))
 	    {
-	      if (p->type != XML_ELEMENT_NODE)
-		continue;
-
 	      if (function_tdecl_sptr f =
 		  build_function_tdecl(ctxt, p,
 				       /*add_to_current_scope=*/true))
@@ -5160,11 +5133,10 @@ build_function_tdecl(read_context& ctxt,
   ctxt.push_decl_to_current_scope(fn_tmpl_decl, add_to_current_scope);
 
   unsigned parm_index = 0;
-  for (xmlNodePtr n = node->children; n; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
       if (template_parameter_sptr parm =
 	  build_template_parameter(ctxt, n, parm_index, fn_tmpl_decl))
 	{
@@ -5224,11 +5196,10 @@ build_class_tdecl(read_context&	ctxt,
   ctxt.push_decl_to_current_scope(class_tmpl, add_to_current_scope);
 
   unsigned parm_index = 0;
-  for (xmlNodePtr n = node->children; n; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
       if (template_parameter_sptr parm=
 	  build_template_parameter(ctxt, n, parm_index, class_tmpl))
 	{
@@ -5343,11 +5314,10 @@ build_type_composition(read_context&		ctxt,
   ctxt.push_decl_to_current_scope(dynamic_pointer_cast<decl_base>(result),
 				  /*add_to_current_scope=*/true);
 
-  for (xmlNodePtr n = node->children; n; n = n->next)
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
     {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
       if ((composed_type =
 	   build_pointer_type_def(ctxt, n,
 				  /*add_to_current_scope=*/true))
@@ -5473,18 +5443,15 @@ build_template_tparameter(read_context&	ctxt,
 
   // Go parse template parameters that are children nodes
   int parm_index = 0;
-  for (xmlNodePtr n = node->children; n; n = n->next)
-    {
-      if (n->type != XML_ELEMENT_NODE)
-	continue;
-
-      if (shared_ptr<template_parameter> p =
-	  build_template_parameter(ctxt, n, parm_index, result))
-	{
-	  result->add_template_parameter(p);
-	  ++parm_index;
-	}
-    }
+  for (xmlNodePtr n = xmlFirstElementChild(node);
+       n;
+       n = xmlNextElementSibling(n))
+    if (shared_ptr<template_parameter> p =
+	build_template_parameter(ctxt, n, parm_index, result))
+      {
+	result->add_template_parameter(p);
+	++parm_index;
+      }
 
   if (result)
     {
  

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.

Thanks for the review!

[...]

Cheers,
  

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
>