From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id D1D7C385AC1B for ; Tue, 1 Feb 2022 16:01:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D1D7C385AC1B Received: from mail-il1-f200.google.com (mail-il1-f200.google.com [209.85.166.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-553-P4tY-xZ3M_KbYwx6Q8fybQ-1; Tue, 01 Feb 2022 11:01:04 -0500 X-MC-Unique: P4tY-xZ3M_KbYwx6Q8fybQ-1 Received: by mail-il1-f200.google.com with SMTP id h8-20020a92c088000000b002bc03432559so7007097ile.7 for ; Tue, 01 Feb 2022 08:01:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:organization:in-reply-to :content-transfer-encoding; bh=ek+/ciOsrSvhLdAVn1ymt7bSimC+pIkiy0UKIAe1TnQ=; b=Ozk13okbbtqKXhdr0QoNs69PB1o8cde2sUH8kuH042jPCcpaqBdxLK1t48Vsqa/KkO EfRpnKM+J/0hp0RWGZvue0u9+sVkD4Rykn2Hi/qQTk4D8yabhU7aOok9FaBuIJKDS1OL RbcD4sDuSOpYRi8KO/C5Xmbj+Z314l+sLO1KyZY5L6m+pyX1RNTPpklx9RR9oDYIMXnh 9mnfh2Q+13dHpjPB8mFbxAE+X9OARjkbC03S/24dHvsbq4ZyPWLdA0ACRqv3IDzdFqM1 DHu5MuGicC5A+h1Ib+uPXPKVbK/uHXHONS7VugvaLnR0o2lqg4TROhOm2eUxfV81oI3T 4qIw== X-Gm-Message-State: AOAM533J2v9+b978s6r4Y9+09OLqSVwAZ9Ehc+1hE8TczC1IWPTsxcZO XjSkiMmVrDAIdLQAAtZIny5ouI8m9Tq4sbh9BYEMynq4cZsrtn44AFV1Bn0oZE3/FjuMq1x+LqA RaJoRbOxMVpri+Q4NJNWV X-Received: by 2002:a05:6602:2e87:: with SMTP id m7mr13764219iow.204.1643731263037; Tue, 01 Feb 2022 08:01:03 -0800 (PST) X-Google-Smtp-Source: ABdhPJxLx67IcdidqLq/c6dTznbau3m5Aa+rWhObHkB/6Yi0WMfVk3cIb9hdTOErTUxXliQtIq70wA== X-Received: by 2002:a05:6602:2e87:: with SMTP id m7mr13764208iow.204.1643731262817; Tue, 01 Feb 2022 08:01:02 -0800 (PST) Received: from [192.168.0.241] (135-23-175-80.cpe.pppoe.ca. [135.23.175.80]) by smtp.gmail.com with ESMTPSA id m1sm20674852ilu.87.2022.02.01.08.00.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 01 Feb 2022 08:01:00 -0800 (PST) Message-ID: <2dbe3ee2-0f4e-c0f7-afe7-1e5d9060048e@redhat.com> Date: Tue, 1 Feb 2022 11:00:56 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH 1/2] localedef: Fix handling of empty mon_decimal_point To: Florian Weimer Cc: libc-alpha@sourceware.org, michael.hudson@canonical.com References: <20220131053442.3995804-1-carlos@redhat.com> <20220131053442.3995804-2-carlos@redhat.com> <87wnie4xm4.fsf@oldenburg.str.redhat.com> From: Carlos O'Donell Organization: Red Hat In-Reply-To: <87wnie4xm4.fsf@oldenburg.str.redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Feb 2022 16:01:09 -0000 On 2/1/22 06:47, Florian Weimer wrote: > * Carlos O'Donell: > >> The handling of mon_decimal_point is incorrect when it comes to >> handling the empty "" value. The existing parser in monetary_read() >> will correctly handle setting the non-wide-character value and the >> wide-character value e.g. STR_ELEM_WC(mon_decimal_point) if they are >> set in the locale definition. However, in monetary_finish() we have >> conflicting TEST_ELEM() which sets a default value (if the locale >> definition doesn't include one), and subsequent code which looks for >> mon_decimal_point to be NULL to issue a specific error message and set >> the defaults. The latter is unused because TEST_ELEM() always sets a >> default. The simplest solution is to remove the TEST_ELEM() check, >> and allow the existing check to look to see if mon_decimal_point is >> NULL and set an appropriate default. The final fix is to move the >> setting of mon_decimal_point_wc so it occurs only when >> mon_decimal_point is being set to a default, keeping both values >> consistent. There is no way to tell the difference between >> mon_decimal_point_wc having been set to the empty string and not >> having been defined at all, for that distinction we must use >> mon_decimal_point being NULL or "", and so we must logically set >> the default together with mon_decimal_point. >> >> Lastly, there are more fixes similar to this that could be made to >> ld-monetary.c, but we avoid that in order to fix just the code >> required for mon_decimal_point, which impacts the ability for C.UTF-8 >> to set mon_decimal_point to "", since without this fix we end up with >> an inconsistent setting of mon_decimal_point set to "", but >> mon_decimal_point_wc set to "." which is incorrect. >> >> Tested on x86_64 and i686 without regression. >> --- >> locale/programs/ld-monetary.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/locale/programs/ld-monetary.c b/locale/programs/ld-monetary.c >> index 277b9ff042..3b0412b405 100644 >> --- a/locale/programs/ld-monetary.c >> +++ b/locale/programs/ld-monetary.c >> @@ -207,7 +207,6 @@ No definition for %s category found"), "LC_MONETARY"); >> >> TEST_ELEM (int_curr_symbol, ""); >> TEST_ELEM (currency_symbol, ""); >> - TEST_ELEM (mon_decimal_point, "."); >> TEST_ELEM (mon_thousands_sep, ""); >> TEST_ELEM (positive_sign, ""); >> TEST_ELEM (negative_sign, ""); >> @@ -257,6 +256,7 @@ not correspond to a valid name in ISO 4217 [--no-warnings=intcurrsym]"), >> record_error (0, 0, _("%s: field `%s' not defined"), >> "LC_MONETARY", "mon_decimal_point"); >> monetary->mon_decimal_point = "."; >> + monetary->mon_decimal_point_wc = L'.'; >> } >> else if (monetary->mon_decimal_point[0] == '\0' && ! be_quiet && ! nothing) >> { >> @@ -264,8 +264,6 @@ not correspond to a valid name in ISO 4217 [--no-warnings=intcurrsym]"), >> %s: value for field `%s' must not be an empty string"), >> "LC_MONETARY", "mon_decimal_point"); >> } >> - if (monetary->mon_decimal_point_wc == L'\0') >> - monetary->mon_decimal_point_wc = L'.'; >> >> if (monetary->mon_grouping_len == 0) >> { > > I have verified that this does not change the localedef output for the > existing locales created by install-locale-files. > > I think we need further cleanups in the comments and checks (which were > coped from LC_NUMERIC, but should not apply to LC_MONETARY). But I > think we can release with this version. I filed this bug to track that: Bug 28845 - ld-monetary.c should be updated to match ISO C and other standards. https://sourceware.org/bugzilla/show_bug.cgi?id=28845 Thanks for the review! > Reviewed-by: Florian Weimer > > Thanks, > Florian > -- Cheers, Carlos.