From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id 73DA5384241F for ; Mon, 1 Feb 2021 14:21:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 73DA5384241F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mark@klomp.org Received: from tarox.wildebeest.org (tarox.wildebeest.org [172.31.17.39]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id AF3423001554; Mon, 1 Feb 2021 15:21:44 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 6B381400194B; Mon, 1 Feb 2021 15:21:44 +0100 (CET) Message-ID: Subject: Re: Remove nested functions from readelf.c From: Mark Wielaard To: tbaeder@redhat.com, elfutils-devel@sourceware.org Date: Mon, 01 Feb 2021 15:21:44 +0100 In-Reply-To: <20210108081633.2202592-1-tbaeder@redhat.com> References: <20210108081633.2202592-1-tbaeder@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Feb 2021 14:21:47 -0000 On Fri, 2021-01-08 at 09:16 +0100, Timm B=C3=A4der via Elfutils-devel wrote= : > here another round for src/readelf.c. I think they are simple, but I'm > not happy with the advance_pc() commit. I'm open for suggestions here > but I can't come up with something better right now. I'll keep looking > in to that in the meantime. Yeah, the advance_pc function is really why you want nested functions in the first place IMHO. Passing around the captured state as 6 extra arguments seems to not really make the code much clearer. Especially because on its own it isn't immediately clear what the code is about or that it is to be used as part of the special opcode or DW_LNS_advance_pc opcode (where DW_LNS_const_add_pc acts like special opcode 255) handling. It might be helpful to spell out in a comment to the function which part of the DWARF4 6.2.5.1 Special Opcodes handling the advance_pc function is responsible for. But honestly in this case I think just keeping the nested function is the cleanest way to handle this. Cheers, Mark