From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121455 invoked by alias); 27 Nov 2019 17:42:50 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 121446 invoked by uid 89); 27 Nov 2019 17:42:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=substantial, guessing, H*F:D*br X-HELO: smtpout1.mo803.mail-out.ovh.net Date: Wed, 27 Nov 2019 17:42:00 -0000 From: "Gabriel F. T. Gomes" To: Paul E Murphy CC: Subject: Re: [PATCH v2 23/30] Refactor *cvt functions implementation (5/5) Message-ID: <20191127144234.05336502@tereshkova> In-Reply-To: References: <20191025153410.15405-1-gabriel@inconstante.net.br> <20191025153410.15405-24-gabriel@inconstante.net.br> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Ovh-Tracer-Id: 13084364295123750456 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedufedrudeihedguddtfecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjqdffgfeufgfipdevjffgvefmvefgnecuuegrihhlohhuthemucehtddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenucfjughrpeffhffvuffkjghfofggtgfgihesthejfedtredtvdenucfhrhhomhepfdfirggsrhhivghlucfhrdcuvfdrucfiohhmvghsfdcuoehgrggsrhhivghlsehinhgtohhnshhtrghnthgvrdhnvghtrdgsrheqnecukfhppedtrddtrddtrddtpddujeelrddukedvrdduheegrddutddtnecurfgrrhgrmhepmhhouggvpehsmhhtphdqohhuthdphhgvlhhopefgigegrdgvmhhprdhlohgtrghlpdhinhgvtheptddrtddrtddrtddpmhgrihhlfhhrohhmpehgrggsrhhivghlsehinhgtohhnshhtrghnthgvrdgvthhirdgsrhdprhgtphhtthhopehlihgstgdqrghlphhhrgesshhouhhrtggvfigrrhgvrdhorhhgnecuvehluhhsthgvrhfuihiivgeptd X-SW-Source: 2019-11/txt/msg00902.txt.bz2 On Thu, 14 Nov 2019, Paul E Murphy wrote: > >I think the non-trivial changes here to fixup the shuffling and the >preceding four patches should be squashed and resent as a single patch >to the list. I am guessing they should squash into a somewhat readable >patch self contained patch. I guess you're right. I'll send them squashed in v3. >The slightly more substantial changes needed to support the new ldbl >would be more easily reviewed as a separate patch (e.g replacing the >__APPEND macro and reworking cvt_symbol) Agreed. >On 10/25/19 10:34 AM, Gabriel F. T. Gomes wrote: >> From: "Gabriel F. T. Gomes" >> >> -#include "efgcvt.c" > >I see some noise in the refactoring :). (and in efgcvt_r.c), :( >> #if LONG_DOUBLE_COMPAT (libc, GLIBC_2_0) >> -# define cvt_symbol(symbol) \ >> - cvt_symbol_1 (libc, __APPEND (FUNC_PREFIX, symbol), \ >> - APPEND (FUNC_PREFIX, symbol), GLIBC_2_4) >> -# define cvt_symbol_1(lib, local, symbol, version) \ >> +# define cvt_symbol(local, symbol) \ >> libc_hidden_def (local) \ >> - versioned_symbol (lib, local, symbol, version) >> + versioned_symbol (libc, local, symbol, GLIBC_2_4) > >This isn't a bug with the patch, but should the version tested in >LONG_DOUBLE_COMPAT match that passed into versioned_symbol? We test against GLIBC_2_0, because that's the version that introduced q{efg}cvt symbols for the first time. The LONG_DOUBLE_COMPAT macro then expands it to SHLIB_COMPAT with GLIBC_2_0 in the 'introduced' argument and LONG_DOUBLE_COMPAT_VERSION in the 'obsoleted' argument. I think it would be OK to change GLIBC_2_4 to LONG_DOUBLE_COMPAT_VERSION (maybe that would make it easier to read). For completeness, on powerpc64, replacing GLIBC_2_0 with GLIBC_2_4 makes test cases fail to build, e.g.: misc/./tst-efgcvt-template.c:214: undefined reference to `qfcvt_r' .../misc/tst-ldbl-efgcvt.o:(.toc+0x0): undefined reference to `qecvt' .../misc/tst-ldbl-efgcvt.o:(.toc+0x8): undefined reference to `qfcvt' .../misc/tst-ldbl-efgcvt.o:(.toc+0x10): undefined reference to `qecvt_r' .../misc/tst-ldbl-efgcvt.o:(.toc+0x18): undefined reference to `qfcvt_r' collect2: error: ld returned 1 exit status >The two non-trivial versioning macros get duplicated in two places. Is >it possible to unify the four very similar instances into a single >shared macro? Before this refactoring, the macro definition for {efg}cvt and q{efg}cvt was defined in a single file, but I failed to see any benefit in it (it relied on the LONG_DOUBLE_CVT macro and provided completely different definitions for each case (long double and double). Likewise for the sharing between {efg}cvt_r and q{efg}cvt_r. As for the differences between {efg}cvt and {efg}cvt_r, they seem too large to allow for the sharing of code (one uses weak_symbol where the other uses strong_symbol; only [q]{efg}cvt_r require hidden_def). Likewise for the long double versions. I think a single definition for all cases would make the code harder to read (maybe it would make it easier to maintain, but I actually find it very confusing and hard to maintain when the macros depend on too many things).