From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua1-x92b.google.com (mail-ua1-x92b.google.com [IPv6:2607:f8b0:4864:20::92b]) by sourceware.org (Postfix) with ESMTPS id 19F423858C27 for ; Tue, 20 Apr 2021 06:53:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 19F423858C27 Received: by mail-ua1-x92b.google.com with SMTP id j8so9291743uak.2 for ; Mon, 19 Apr 2021 23:53:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aBAjFtlupHaJH2nak8/U8dZiALlHKhwtNBwTmtB3TCU=; b=lsPf/HZ+WW8Piw0S6juvRw4npFIQrs5jIuhAw4yxVG6mRaYtG04/hFkBf8JdxlkAKg K6TY9pjKOpU/jr5wWZx/Mx4wt/wDO6DUO1fXtTxyv7xRj+MMOv8j7inAGbIJzzX+se2p On6e/eWIGK0HNQzs0kS8I620D5OC7B2pTbMkPalpR3ItWAVIHqoUlxVMTCqPotBeRyBS YHJnsEW4kj6AG+QD/PN5Me2/Evzf0yEIRbbRMmMgKTj99euCGgyeVIT9Ot3ZkygMKEmq TrUIv8dV+ik5VQYzpmhZ2n/GQTarhzWsuehh8PFebzzDtBKs1j1Cdvcf+4FhcMFDTCiB DAjQ== X-Gm-Message-State: AOAM53154B3OIdH2Z/MhEYv/eFySM8tBo3E7owLPIpn7tGE2WZXc4k50 LfVznQQ38it8DI4Ykyq8a7ryaZJG0taPEeDMUvxB4ViNtfk= X-Google-Smtp-Source: ABdhPJynnwm5Sz96yZDwd52J3GK7t28/d2o8WTkjnBhT6+0ZGJ+kf2YY2z9DjIvlcKA1Fr4zNo5o0ajTEblx4P9kGU4= X-Received: by 2002:ab0:60c5:: with SMTP id g5mr10442627uam.5.1618901584393; Mon, 19 Apr 2021 23:53:04 -0700 (PDT) MIME-Version: 1.0 References: <20210331092352.619148-1-gprocida@google.com> <20210331170454.951900-1-gprocida@google.com> <8735w3vdkv.fsf@seketeli.org> <87o8eftz5v.fsf_-_@seketeli.org> <87k0oytmsf.fsf@seketeli.org> <877dkytlqo.fsf_-_@seketeli.org> In-Reply-To: <877dkytlqo.fsf_-_@seketeli.org> From: Giuliano Procida Date: Tue, 20 Apr 2021 07:52:52 +0100 Message-ID: Subject: Re: [PATCH 3/3] reader: Use xmlFirstElementChild/xmlNextElementSibling to iterate over children elements To: Dodji Seketeli Cc: =?UTF-8?Q?Matthias_M=C3=A4nnich?= , Giuliano Procida via Libabigail , kernel-team@android.com X-Spam-Status: No, score=-28.9 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Apr 2021 06:53:15 -0000 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, 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 > --- > 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& 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 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 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(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 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 p = > + build_template_parameter(ctxt, n, parm_index, result)) > + { > + result->add_template_parameter(p); > + ++parm_index; > + } > > if (result) > { > -- > 2.30.0 > > > > -- > Dodji >