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

[Dodji Seketeli <dodji@seketeli.org>]

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)
     {
-- 
2.30.0



-- 
		Dodji