From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id y9xVCHPnu18wTwAAWB0awg (envelope-from ) for ; Mon, 23 Nov 2020 11:46:43 -0500 Received: by simark.ca (Postfix, from userid 112) id 14ACC1F0AB; Mon, 23 Nov 2020 11:46:43 -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 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 B392C1E58E for ; Mon, 23 Nov 2020 11:46:42 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 16DBC38708AD; Mon, 23 Nov 2020 16:46:42 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 77468386F472 for ; Mon, 23 Nov 2020 16:46:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 77468386F472 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 0C9BB1E58E; Mon, 23 Nov 2020 11:46:39 -0500 (EST) Subject: Re: RFA v2: Various enhancements to the fixed-point support implementation To: Joel Brobecker , gdb-patches@sourceware.org References: <1605430184-81335-1-git-send-email-brobecker@adacore.com> <1606043646-140022-1-git-send-email-brobecker@adacore.com> From: Simon Marchi Message-ID: <76abfcda-8f1a-a903-da56-29701cea3272@simark.ca> Date: Mon, 23 Nov 2020 11:46:38 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <1606043646-140022-1-git-send-email-brobecker@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit 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: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-11-22 6:14 a.m., Joel Brobecker wrote: > Hello, > > This is v2 of the following patch series: > > - [RFA v2 1/6] change and rename gmp_string_asprintf to return an > - [RFA v2 2/6] gmp-utils: Convert the read/write methods to using > - [RFA v2 3/6] gdbtypes.h: Get rid of the TYPE_FIXED_POINT_INFO macro > - [RFA v2 4/6] Make fixed_point_type_base_type a method of struct type > - [RFA v2 5/6] Make function fixed_point_scaling_factor a method of > - [RFA v2 6/6] valarith.c: Replace INIT_VAL_WITH_FIXED_POINT_VAL macro > > The only changes I made were to patch #1 and patch #2, following > the commends made by Simon and Pedro: > > - [RFA v2 1/6] change and rename gmp_string_asprintf to return an > > Rename the function to gmp_string_printf as recommended, > and use the suggestion to rely on gmp_vsnprintf to only > allocate the string buffer once. > > - [RFA v2 2/6] gmp-utils: Convert the read/write methods to using > > Improve the patch using either the form {ptr, len} to construct > the gdb_byte array, or else gdb::make_array_view. > > The other patches are just plain rebases. > > Each patch was individually tested on x86_64-linux, with no regression. > > There are still a number of comments which were made by Simon and > Pedro (xfail for m2 and pascal, NEWS entries, selftest error > using Ubuntu's GMP, etc), and I will address those next, in > separate submissions. > > Note that I almost dropped patch #5 (turning fixed_point_scaling_factor > from function to method), since Simon replied to the discussion > about this topic that the the current form (a separate function) was fine. > In particular, I tried to think about it in a possible future context > where types are a tree of classes rather than one class with a type-code. > In that situation, the fixed_point_scaling_factor method would logically > be "attached" to the fixed point type class. This in turn means that > ranges of fixed point types would not have this method, forcing users > to have to call fixed_point_type_base_type first before they can query > the fixed_point_scaling_factor method. In other words, because > the functionality applies to two types where one is not the child of > the other (in terms of class inheritance), a functioned did seem better. > > However, one could argue that the fixed_point_type_base_type method > is in exactly the same boat: It applies to both range types and > fixed point. So, in the name of being consistent, this patch series > keeps them both. My thinking process is based on a hypothetical > scenario, which I'm not even sure ever got discussed here anyways. > > That being said, now that I write my thoughts, for the case of > fixed_point_type_base_type, if we did have a hierarchy of classes, > I could perhaps see how all types would have a "base_type" method > whose default implementation would simply return the type itself, > and were range types would return the target type. In that scenario, > the two methods are no longer in the same boat, and we can think > of dropping the fixed_point_scaling_factor patch if we wanted to. > > I do not have a strong opinion and thus can easily drop any patch. > The reason I kept the change in the end is that it just seemed > more logical to act on what is today, rather than decide based on > what the code might be tomorrow. I also don't know how it would look if/when we convert this to a hierarchy of classes, and I also prefer that we do what makes sense today rather than act on speculations of what this could become. We can always change things later, it's not an API that we have to keep stable. Following my changes, there are already methods in struct type that apply to only some type codes, like bounds/set_gounds. So I think it's ok to make fixed_point_scaling_factor a method too, the same solution will apply to all of them (deciding whether it's a virtual method only implemented by some classes, or if it's a non-virtual method on some specific classes). In any case, I think the series looks good. I let you decide if you want to make fixed_point_scaling_factor a method or not, it doesn't make a big difference in the end. Simon