From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id K4crFKFt8V/yeAAAWB0awg (envelope-from ) for ; Sun, 03 Jan 2021 02:09:21 -0500 Received: by simark.ca (Postfix, from userid 112) id 45D1E1F0AA; Sun, 3 Jan 2021 02:09:21 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI,RDNS_NONE, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id ED23D1E965 for ; Sun, 3 Jan 2021 02:09:20 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2EC5E3857004; Sun, 3 Jan 2021 07:09:20 +0000 (GMT) Received: from rock.gnat.com (rock.gnat.com [205.232.38.15]) by sourceware.org (Postfix) with ESMTP id CDF6C3857004 for ; Sun, 3 Jan 2021 07:09:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CDF6C3857004 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=brobecker@adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id A79EC117226; Sun, 3 Jan 2021 02:09:17 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id r3kjtg4QKJqq; Sun, 3 Jan 2021 02:09:17 -0500 (EST) Received: from float.home (localhost.localdomain [127.0.0.1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id 4A82B11721B; Sun, 3 Jan 2021 02:09:17 -0500 (EST) Received: by float.home (Postfix, from userid 1000) id 6A300A1608; Sun, 3 Jan 2021 11:09:12 +0400 (+04) Date: Sun, 3 Jan 2021 11:09:12 +0400 From: Joel Brobecker To: Tom Tromey Subject: Re: [PATCH 073/203] Introduce class operation Message-ID: <20210103070912.GA363503@adacore.com> References: <20210101214723.1784144-1-tom@tromey.com> <20210101214723.1784144-74-tom@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210101214723.1784144-74-tom@tromey.com> X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Hi Tom, > This patch introduces class operation, the new base class for all > expression operations. > > In the new approach, an operation is simply a class that presents a > certain interface. Operations own their operands, and management is > done via unique_ptr. > > The operation interface is largely ad hoc, based on the evolution of > expression handling in GDB. Parts (for example, > evaluate_with_coercion) are probably redundant; however I took this > approach to try to avoid mixing different kinds of refactorings. > > In some specific situations, rather than add a generic method across > the entire operation class hierarchy, I chose instead to use > dynamic_cast and specialized methods on certain concrete subclasses. > This will appear in some subsequent patches. > > One goal of this work is to avoid the kinds of easy-to-make errors > that affected the old implementation. To this end, some helper > subclasses are also added here. These helpers automate the > implementation of the 'dump', 'uses_objfile', and 'constant_p' > methods. Nearly every concrete operation that is subsequently added > will use these facilities. (Note that the 'dump' implementation is > only outlined here, the body appears in the next patch.) > > gdb/ChangeLog > 2021-01-01 Tom Tromey > > * expression.h (expr::operation): New class. > (expr::make_operation): New function. > (expr::operation_up): New typedef. > * expop.h: New file. > * eval.c (operation::evaluate_for_cast) > (operation::evaluate_for_address, operation::evaluate_for_sizeof): > New methods. > * ax-gdb.c (operation::generate_ax): New method. Thanks for this! I don't have much comment to share on this, at least not just yet until I have had a chance to see this new API in action in the rest of the patch series. I do have one small-tiny-mini suggestion: It's about: > +typedef std::unique_ptr operation_up; FWIW, the connection between "up" and the rest of the declaration wasn't immediately obvious to me. I think this is because "up" is an english word, and spelled like that, all in lowercase, that's really what my brain keeps analyzing it as first. It takes an effort to adjust my thinking against this bias. It's not a problem for me if we decide to keep this like that, but what do you think about about renaming this to something like "operation_uptr"? I think it'll make the intention more immediately clear to the reader. -- Joel