From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x330.google.com (mail-wm1-x330.google.com [IPv6:2a00:1450:4864:20::330]) by sourceware.org (Postfix) with ESMTPS id 49445385BF9D for ; Tue, 4 May 2021 11:14:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 49445385BF9D Received: by mail-wm1-x330.google.com with SMTP id l24-20020a7bc4580000b029014ac3b80020so1016741wmi.1 for ; Tue, 04 May 2021 04:14:09 -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=/qsbzRaqIlf6krmkftyFoP0TczNIyYPMZSvRvpDALys=; b=hq9nDkVA/HFoupKGtwRoPPjKOtPyn4iJZVx7DAbuWaxcm3mW0VfEzsF0MuCwKq704y xtB/f9ikmt0CdcJvcD5pj7dLA30QZ4LkyVhV8wXI3/SoPvqokNTb7H5Pyc8vLbp19I6g IB+eMGDQT7yT6AbkKdHssZra7fl42+Tkt0WeNvob5SzWMD7kK4XG5Ww/89hbXkXMQ5gE kqBQEdnuA7w920CWe7yMKNBQxhgkC9tYsv4n0ILVp471xc89NXRGGcMBWiqfq94DOjc/ vKNo2jsafz4ekW4nLsIc9R4n1dNVU2Kk/eQOXMTTqPF/0DdvrKGZ4UUaNLAFG2k1nmIY +sEg== X-Gm-Message-State: AOAM530Q+tRjwX/fWb6ph2ZUS50ByKdZC1ORlmqgqAUnGSwiVm7fRt/K 7pFrbCBeovCjYF6q2YRsiiLWr3vozMl42j+00P+5NX/Pno4+xw== X-Google-Smtp-Source: ABdhPJx0QuMgBvh5uM1pH2D/y9GBLCw+ZTkqCeMw8elIyuOQKCBAuHhHHC/mhUZYR4oVevB+o6bU8lTybX8R7zCmCDA= X-Received: by 2002:a1c:f402:: with SMTP id z2mr3448965wma.21.1620126848190; Tue, 04 May 2021 04:14:08 -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> <8735vlthjm.fsf@seketeli.org> <87tunjsv63.fsf@seketeli.org> In-Reply-To: <87tunjsv63.fsf@seketeli.org> From: Giuliano Procida Date: Tue, 4 May 2021 12:13:33 +0100 Message-ID: Subject: Re: [PATCH 3/3] reader: Use xmlFirstElementChild/xmlNextElementSibling to iterate over children elements To: Dodji Seketeli Cc: kernel-team@android.com, Giuliano Procida via Libabigail , =?UTF-8?Q?Matthias_M=C3=A4nnich?= X-Spam-Status: No, score=-22.0 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, 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" Content-Transfer-Encoding: quoted-printable 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, 04 May 2021 11:14:11 -0000 Hi there. On Mon, 3 May 2021 at 16:18, Dodji Seketeli wrote: > Hello, > > [...] > > Dodji Seketeli a =C3=A9crit: > > > Hello Giuliano, > > > > Giuliano Procida a =C3=A9crit: > > > > > >> 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 > > 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 > > 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=3D26591#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 >