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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 1F4DF383F878 for ; Fri, 2 Oct 2020 21:05:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1F4DF383F878 Received: from mail-pg1-f198.google.com (mail-pg1-f198.google.com [209.85.215.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-401-E_Mh4XtUPcaU5wetVDz-3w-1; Fri, 02 Oct 2020 17:04:57 -0400 X-MC-Unique: E_Mh4XtUPcaU5wetVDz-3w-1 Received: by mail-pg1-f198.google.com with SMTP id v187so1813961pgv.0 for ; Fri, 02 Oct 2020 14:04:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=v9kVvxrlBcSFraF9PLxzbJbIKcRS3oV+tq3Btpb1Vvg=; b=N424Y8Yqt+g/LG5GTkBMuRSik2gHODurVg6MYYjh4hTbxWGohk1PUUqyysDJkk5VDI Q9HsS3Yw8hgGfSAT2fjRp2E/HcmarsS4S06WTIgZ9sEvsRjspHjqQkrXTq8YFj/suuSu VzryZADgmjCDmpwfpm0XxdLhfq3MFfZ/dHzffPNdXWWnECZRbq2kRNwiR5iJDHxEev+o u84aqRGlA7vX1NgR0FDzXUZwZyk2O5HqT14fx0jIi39xo3W/IQ3chXalXRFVVn5bTvaw g9ElWc+UEYGv6Cz4MX+RSs/u4eZxOEWWrIUv0bXFNuZfD7z2sI/y8VkuTy+OJ5DCceA+ XPiA== X-Gm-Message-State: AOAM530hc5fSuajXOgaEUUWv8uj6JoJ1cxjwbyyN3XMuxS1DGMOUQrKZ xiyC8sMHqui2pI7H7XSFwi6zpslcomQ8nbvCJJoP6ojiP76iS22GRxR6uwEDsz9bJq9uhoHuFFE LDW/Tnm2GiL6ILBad136hng0gjduChE4= X-Received: by 2002:aa7:9157:0:b029:142:2501:3984 with SMTP id 23-20020aa791570000b029014225013984mr4620658pfi.73.1601672696462; Fri, 02 Oct 2020 14:04:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxUW2S/bgSA9ZS2Zbhn7aIzaiExrYWyDPqm1VFiPnBvrGJi54rYEu04usKZ4vHSBfAl4Xiy2qyaAqjLlq4Uwf4= X-Received: by 2002:aa7:9157:0:b029:142:2501:3984 with SMTP id 23-20020aa791570000b029014225013984mr4620635pfi.73.1601672696189; Fri, 02 Oct 2020 14:04:56 -0700 (PDT) MIME-Version: 1.0 References: <20201001141753.6657-1-torbjorn.svensson@st.com> <20201001141753.6657-4-torbjorn.svensson@st.com> In-Reply-To: From: Jeff Johnston Date: Fri, 2 Oct 2020 17:04:45 -0400 Message-ID: Subject: Re: [PATCH 3/3] libc: Replace one letter member names in __tzrule_struct To: Torbjorn SVENSSON Cc: Newlib X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Newlib mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Oct 2020 21:05:02 -0000 On Fri, Oct 2, 2020 at 3:36 AM Torbjorn SVENSSON wrote: > Hello! > > Thanks for the feedback Jeff. > > Regarding the alternative to using "__" prefix for the members; would it > be better to move line 105-123 from newlib/libc/include/time.h to > newlib/libc/time/local.h, thus not exposing it in the API? > Yes. It was one of the alternatives I mentioned (hiding the struct from users of time.h). The struct is hidden in glibc and should have been hidden likewise in newlib. The lines in question are: > typedef struct __tzrule_struct > { > char ch; > int m; > int n; > int d; > int s; > time_t change; > long offset; /* Match type of _timezone. */ > } __tzrule_type; > > typedef struct __tzinfo_struct > { > int __tznorth; > int __tzyear; > __tzrule_type __tzrule[2]; > } __tzinfo_type; > > __tzinfo_type *__gettzinfo (void); > > > > Would this change be easier to get accepted? If this approach is better, = I > guess the single letter member names can be kept too=E2=80=A6 > While I took another look at the change, I also noticed that the struct > definition (not usage) is also present in these files: > newlib/libc/sys/linux/include/time.h > newlib/libc/sys/phoenix/include/time.h > > I=E2=80=99m not sure in what situations those files are used and when the= time.h > file that I modified is used, but I guess they should be in sync regardle= ss. > > Thanks > Torbj=C3=B6rn > > From: Jeff Johnston > Sent: den 2 oktober 2020 01:21 > To: Torbjorn SVENSSON > Cc: Newlib > Subject: Re: [PATCH 3/3] libc: Replace one letter member names in > __tzrule_struct > > Hello, while I fully agree there is an issue (the struct was due to my > initial check-in in 2005 based on glibc), the change will break API and > thus technically requires a major release bump of newlib. I would prefer > to wait for something else > to require a major bump before making such a change. In such a case, I > also believe that the patch should either use double-underscores for the > field names (e.g. __month) or hide the struct from regular users of time.= h. > > -- Jeff J. > > On Thu, Oct 1, 2020 at 10:19 AM Torbj=C3=B6rn SVENSSON via Newlib newlib@sourceware.org> wrote: > As discussed in GCC bug 97088 > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D97088), parameters in > prototypes of library functions should use reserved names, or no name > at all. > > This patch replaces 'm', 'n', 'd' and 's' members in > 'struct __tzrule_struct' to avoid possible clashes with user code in > case someone uses before including Newlib's time.h (or uses some > other conflicting definition) > > Signed-off-by: Torbj=C3=B6rn SVENSSON > --- > newlib/libc/include/time.h | 8 ++++---- > newlib/libc/time/tzcalc_limits.c | 14 +++++++------- > newlib/libc/time/tzset_r.c | 22 +++++++++++----------- > 3 files changed, 22 insertions(+), 22 deletions(-) > > diff --git a/newlib/libc/include/time.h b/newlib/libc/include/time.h > index 3031590b4..6a540537f 100644 > --- a/newlib/libc/include/time.h > +++ b/newlib/libc/include/time.h > @@ -105,10 +105,10 @@ void _tzset_r (struct _reent *); > typedef struct __tzrule_struct > { > char ch; > - int m; > - int n; > - int d; > - int s; > + int month; /* Month of year if ch=3DM */ > + int week; /* Week of month if ch=3DM */ > + int day; /* Day of week if ch=3DM, day of year if ch=3DJ or ch=3DD */ > + int secs; /* Time of day in seconds */ > time_t change; > long offset; /* Match type of _timezone. */ > } __tzrule_type; > diff --git a/newlib/libc/time/tzcalc_limits.c > b/newlib/libc/time/tzcalc_limits.c > index e0ea6549c..b2163ed3d 100644 > --- a/newlib/libc/time/tzcalc_limits.c > +++ b/newlib/libc/time/tzcalc_limits.c > @@ -34,13 +34,13 @@ __tzcalc_limits (int year) > if (tz->__tzrule[i].ch =3D=3D 'J') > { > /* The Julian day n (1 <=3D n <=3D 365). */ > - days =3D year_days + tz->__tzrule[i].d + > - (isleap(year) && tz->__tzrule[i].d >=3D 60); > + days =3D year_days + tz->__tzrule[i].day + > + (isleap(year) && tz->__tzrule[i].day >=3D 60); > /* Convert to yday */ > --days; > } > else if (tz->__tzrule[i].ch =3D=3D 'D') > - days =3D year_days + tz->__tzrule[i].d; > + days =3D year_days + tz->__tzrule[i].day; > else > { > const int yleap =3D isleap(year); > @@ -49,15 +49,15 @@ __tzcalc_limits (int year) > > days =3D year_days; > > - for (j =3D 1; j < tz->__tzrule[i].m; ++j) > + for (j =3D 1; j < tz->__tzrule[i].month; ++j) > days +=3D ip[j-1]; > > m_wday =3D (EPOCH_WDAY + days) % DAYSPERWEEK; > > - wday_diff =3D tz->__tzrule[i].d - m_wday; > + wday_diff =3D tz->__tzrule[i].day - m_wday; > if (wday_diff < 0) > wday_diff +=3D DAYSPERWEEK; > - m_day =3D (tz->__tzrule[i].n - 1) * DAYSPERWEEK + wday_diff; > + m_day =3D (tz->__tzrule[i].week - 1) * DAYSPERWEEK + wday_diff; > > while (m_day >=3D ip[j-1]) > m_day -=3D DAYSPERWEEK; > @@ -67,7 +67,7 @@ __tzcalc_limits (int year) > > /* store the change-over time in GMT form by adding offset */ > tz->__tzrule[i].change =3D days * SECSPERDAY + > - tz->__tzrule[i].s + tz->__tzrule[i].offset; > + tz->__tzrule[i].secs + tz->__tzrule[i].offset; > } > > tz->__tznorth =3D (tz->__tzrule[0].change < tz->__tzrule[1].change); > diff --git a/newlib/libc/time/tzset_r.c b/newlib/libc/time/tzset_r.c > index 9e0cf834b..7117b51e6 100644 > --- a/newlib/libc/time/tzset_r.c > +++ b/newlib/libc/time/tzset_r.c > @@ -115,9 +115,9 @@ _tzset_unlocked_r (struct _reent *reent_ptr) > return; > > tz->__tzrule[i].ch =3D 'M'; > - tz->__tzrule[i].m =3D m; > - tz->__tzrule[i].n =3D w; > - tz->__tzrule[i].d =3D d; > + tz->__tzrule[i].month =3D m; > + tz->__tzrule[i].week =3D w; > + tz->__tzrule[i].day =3D d; > > tzenv +=3D n; > } > @@ -142,22 +142,22 @@ _tzset_unlocked_r (struct _reent *reent_ptr) > if (i =3D=3D 0) > { > tz->__tzrule[0].ch =3D 'M'; > - tz->__tzrule[0].m =3D 3; > - tz->__tzrule[0].n =3D 2; > - tz->__tzrule[0].d =3D 0; > + tz->__tzrule[0].month =3D 3; > + tz->__tzrule[0].week =3D 2; > + tz->__tzrule[0].day =3D 0; > } > else > { > tz->__tzrule[1].ch =3D 'M'; > - tz->__tzrule[1].m =3D 11; > - tz->__tzrule[1].n =3D 1; > - tz->__tzrule[1].d =3D 0; > + tz->__tzrule[1].month =3D 11; > + tz->__tzrule[1].week =3D 1; > + tz->__tzrule[1].day =3D 0; > } > } > else > { > tz->__tzrule[i].ch =3D ch; > - tz->__tzrule[i].d =3D d; > + tz->__tzrule[i].day =3D d; > } > > tzenv =3D end; > @@ -172,7 +172,7 @@ _tzset_unlocked_r (struct _reent *reent_ptr) > if (*tzenv =3D=3D '/') > sscanf (tzenv, "/%hu%n:%hu%n:%hu%n", &hh, &n, &mm, &n, &ss, &n); > > - tz->__tzrule[i].s =3D ss + SECSPERMIN * mm + SECSPERHOUR * hh; > + tz->__tzrule[i].secs =3D ss + SECSPERMIN * mm + SECSPERHOUR * hh; > > tzenv +=3D n; > } > -- > 2.18.0 >