[v2] libc/time: Move internal newlib tz-structs to local.h

Message ID 20201005125011.28344-1-torbjorn.svensson@st.com
State New
Headers show
Series
  • [v2] libc/time: Move internal newlib tz-structs to local.h
Related show

Commit Message

Corinna Vinschen via Newlib Oct. 5, 2020, 12:50 p.m.
As discussed in GCC bug 97088
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97088), parameters in
prototypes of library functions should use reserved names, or no name
at all.

This patch moves the internal struct __tzrule_struct from the public
API to the internal headerfile newlib/libc/time/local.h.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>

---
 newlib/libc/include/time.h             | 20 --------------------
 newlib/libc/sys/linux/include/time.h   | 20 --------------------
 newlib/libc/sys/phoenix/include/time.h | 17 -----------------
 newlib/libc/time/local.h               | 19 +++++++++++++++++++
 4 files changed, 19 insertions(+), 57 deletions(-)

-- 
2.18.0

Comments

Corinna Vinschen via Newlib Oct. 15, 2020, 6:52 a.m. | #1
Ping!

-----Original Message-----
From: Torbjorn SVENSSON <torbjorn.svensson@st.com> 

Sent: den 5 oktober 2020 14:50
To: newlib@sourceware.org
Cc: Torbjorn SVENSSON <torbjorn.svensson@st.com>; jjohnstn@redhat.com
Subject: [PATCH v2] libc/time: Move internal newlib tz-structs to local.h

As discussed in GCC bug 97088
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97088), parameters in
prototypes of library functions should use reserved names, or no name
at all.

This patch moves the internal struct __tzrule_struct from the public
API to the internal headerfile newlib/libc/time/local.h.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>

---
 newlib/libc/include/time.h             | 20 --------------------
 newlib/libc/sys/linux/include/time.h   | 20 --------------------
 newlib/libc/sys/phoenix/include/time.h | 17 -----------------
 newlib/libc/time/local.h               | 19 +++++++++++++++++++
 4 files changed, 19 insertions(+), 57 deletions(-)

diff --git a/newlib/libc/include/time.h b/newlib/libc/include/time.h
index 3031590b4..ed6cc70fe 100644
--- a/newlib/libc/include/time.h
+++ b/newlib/libc/include/time.h
@@ -102,26 +102,6 @@ void      tzset 	(void);
 #endif
 void      _tzset_r 	(struct _reent *);
 
-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);
-
 /* getdate functions */
 
 #ifdef HAVE_GETDATE
diff --git a/newlib/libc/sys/linux/include/time.h b/newlib/libc/sys/linux/include/time.h
index 5e61d2b65..917a35858 100644
--- a/newlib/libc/sys/linux/include/time.h
+++ b/newlib/libc/sys/linux/include/time.h
@@ -84,26 +84,6 @@ char      *strptime (const char *, const char *, struct tm *);
 void      tzset 	(void);
 void      _tzset_r 	(struct _reent *);
 
-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);
-
 /* getdate functions */
 
 #ifndef _REENT_ONLY
diff --git a/newlib/libc/sys/phoenix/include/time.h b/newlib/libc/sys/phoenix/include/time.h
index 3a9449c77..41fb137e4 100644
--- a/newlib/libc/sys/phoenix/include/time.h
+++ b/newlib/libc/sys/phoenix/include/time.h
@@ -40,23 +40,6 @@ extern char *_tzname[2];
 #define tzname	_tzname
 #endif
 
-typedef struct __tzrule_struct {
-	char ch;
-	int m;
-	int n;
-	int d;
-	int s;
-	time_t change;
-	long offset;
-} __tzrule_type;
-
-typedef struct __tzinfo_struct {
-	int __tznorth;
-	int __tzyear;
-	__tzrule_type __tzrule[2];
-} __tzinfo_type;
-
-__tzinfo_type *__gettzinfo();
 void tzset();
 
 clock_t clock();
diff --git a/newlib/libc/time/local.h b/newlib/libc/time/local.h
index dce51cda2..290e1aee5 100644
--- a/newlib/libc/time/local.h
+++ b/newlib/libc/time/local.h
@@ -38,3 +38,22 @@ void _tzset_unlocked (void);
 void __tz_lock (void);
 void __tz_unlock (void);
 
+typedef struct __tzrule_struct
+{
+  char ch;
+  int m; /* Month of year if ch=M */
+  int n; /* Week of month if ch=M */
+  int d; /* Day of week if ch=M, day of year if ch=J or ch=D */
+  int s; /* Time of day in seconds */
+  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);
-- 
2.18.0
Corinna Vinschen via Newlib Oct. 15, 2020, 10:21 a.m. | #2
On Oct 15 06:52, Torbjorn SVENSSON via Newlib wrote:
> Ping!


Due to vacation I only just checked now.  The patch breaks the Cygwin
build.  Especially __tzinfo_type has to be exposed from a public header
in newlib.

So what about the attached patch instead?


Corinna
From c6ad49622e42b4b80ba5fbad40a0776ec74e9ec5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Torbj=C3=B6rn=20SVENSSON=20via=20Newlib?=

 <newlib@sourceware.org>
Date: Mon, 5 Oct 2020 14:50:13 +0200
Subject: [PATCH] libc/time: Move internal newlib tz-structs into own header
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As discussed in GCC bug 97088
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97088), parameters in
prototypes of library functions should use reserved names, or no name
at all.

This patch moves the internal struct __tzrule_struct to its own
internal header sys/_tz_structs.h.  This is included from newlib's
time code as well as from Cygwin's localtime wrapper.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>

---
 newlib/libc/include/sys/_tz_structs.h    | 24 ++++++++++++++++++++++++
 newlib/libc/include/time.h               | 20 --------------------
 newlib/libc/sys/linux/include/time.h     | 20 --------------------
 newlib/libc/sys/phoenix/include/time.h   | 17 -----------------
 newlib/libc/time/local.h                 |  1 +
 winsup/cygwin/tzcode/localtime_wrapper.c |  1 +
 6 files changed, 26 insertions(+), 57 deletions(-)
 create mode 100644 newlib/libc/include/sys/_tz_structs.h

diff --git a/newlib/libc/include/sys/_tz_structs.h b/newlib/libc/include/sys/_tz_structs.h
new file mode 100644
index 000000000000..9610b06819e1
--- /dev/null
+++ b/newlib/libc/include/sys/_tz_structs.h
@@ -0,0 +1,24 @@
+#ifndef _SYS__TZ_STRUCTS_H_
+#define _SYS__TZ_STRUCTS_H_
+
+typedef struct __tzrule_struct
+{
+  char ch;
+  int m; /* Month of year if ch=M */
+  int n; /* Week of month if ch=M */
+  int d; /* Day of week if ch=M, day of year if ch=J or ch=D */
+  int s; /* Time of day in seconds */
+  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);
+
+#endif /* _SYS__TZ_STRUCTS_H_ */
diff --git a/newlib/libc/include/time.h b/newlib/libc/include/time.h
index 3031590b441c..ed6cc70fec94 100644
--- a/newlib/libc/include/time.h
+++ b/newlib/libc/include/time.h
@@ -102,26 +102,6 @@ void      tzset 	(void);
 #endif
 void      _tzset_r 	(struct _reent *);
 
-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);
-
 /* getdate functions */
 
 #ifdef HAVE_GETDATE
diff --git a/newlib/libc/sys/linux/include/time.h b/newlib/libc/sys/linux/include/time.h
index 5e61d2b65057..917a35858285 100644
--- a/newlib/libc/sys/linux/include/time.h
+++ b/newlib/libc/sys/linux/include/time.h
@@ -84,26 +84,6 @@ char      *strptime (const char *, const char *, struct tm *);
 void      tzset 	(void);
 void      _tzset_r 	(struct _reent *);
 
-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);
-
 /* getdate functions */
 
 #ifndef _REENT_ONLY
diff --git a/newlib/libc/sys/phoenix/include/time.h b/newlib/libc/sys/phoenix/include/time.h
index 3a9449c77540..41fb137e4391 100644
--- a/newlib/libc/sys/phoenix/include/time.h
+++ b/newlib/libc/sys/phoenix/include/time.h
@@ -40,23 +40,6 @@ extern char *_tzname[2];
 #define tzname	_tzname
 #endif
 
-typedef struct __tzrule_struct {
-	char ch;
-	int m;
-	int n;
-	int d;
-	int s;
-	time_t change;
-	long offset;
-} __tzrule_type;
-
-typedef struct __tzinfo_struct {
-	int __tznorth;
-	int __tzyear;
-	__tzrule_type __tzrule[2];
-} __tzinfo_type;
-
-__tzinfo_type *__gettzinfo();
 void tzset();
 
 clock_t clock();
diff --git a/newlib/libc/time/local.h b/newlib/libc/time/local.h
index dce51cda29bf..bfe06e62230d 100644
--- a/newlib/libc/time/local.h
+++ b/newlib/libc/time/local.h
@@ -1,6 +1,7 @@
 /* local header used by libc/time routines */
 #include <_ansi.h>
 #include <time.h>
+#include <sys/_tz_structs.h>
 
 #define SECSPERMIN	60L
 #define MINSPERHOUR	60L
diff --git a/winsup/cygwin/tzcode/localtime_wrapper.c b/winsup/cygwin/tzcode/localtime_wrapper.c
index 3ac8f8cb00ea..4e784480b0f4 100644
--- a/winsup/cygwin/tzcode/localtime_wrapper.c
+++ b/winsup/cygwin/tzcode/localtime_wrapper.c
@@ -11,6 +11,7 @@ details. */
 #include "tz_posixrules.h"
 #include <cygwin/version.h>
 #include <stdlib.h>
+#include <sys/_tz_structs.h>
 
 static NO_COPY SRWLOCK tzset_guard = SRWLOCK_INIT;
 
-- 
2.26.2
Corinna Vinschen via Newlib Oct. 15, 2020, 4:47 p.m. | #3
Hello Corinna,

Thanks for the feedback and I hope that you had a nice vacation!

I haven't run tests with your patch applied, but just reading it should be fine.
I see no reason why "sys/_tz_structs.h" would be included from "bits/stdc++.h", and it is this particular include chain that is causing the problems in the libstdc ++ test suite, so if it works for Cygwin, I'd say go for it!

Kind regards,
Torbjörn

-----Original Message-----
From: Corinna Vinschen <vinschen@redhat.com> 

Sent: den 15 oktober 2020 12:22
To: Torbjorn SVENSSON <torbjorn.svensson@st.com>
Cc: newlib@sourceware.org
Subject: Re: [PATCH v2] libc/time: Move internal newlib tz-structs to local.h

On Oct 15 06:52, Torbjorn SVENSSON via Newlib wrote:
> Ping!


Due to vacation I only just checked now.  The patch breaks the Cygwin
build.  Especially __tzinfo_type has to be exposed from a public header
in newlib.

So what about the attached patch instead?


Corinna
Corinna Vinschen via Newlib Oct. 15, 2020, 5:09 p.m. | #4
I also like this solution, but again as I mentioned earlier, it technically
requires a major release bump due to the removal of API from time.h and
would be best
to wait until we have something else warranting a major release bump.

That said, what do you think Corinna?

-- Jeff J.

On Thu, Oct 15, 2020 at 12:47 PM Torbjorn SVENSSON via Newlib <
newlib@sourceware.org> wrote:

> Hello Corinna,

>

> Thanks for the feedback and I hope that you had a nice vacation!

>

> I haven't run tests with your patch applied, but just reading it should be

> fine.

> I see no reason why "sys/_tz_structs.h" would be included from

> "bits/stdc++.h", and it is this particular include chain that is causing

> the problems in the libstdc ++ test suite, so if it works for Cygwin, I'd

> say go for it!

>

> Kind regards,

> Torbjörn

>

> -----Original Message-----

> From: Corinna Vinschen <vinschen@redhat.com>

> Sent: den 15 oktober 2020 12:22

> To: Torbjorn SVENSSON <torbjorn.svensson@st.com>

> Cc: newlib@sourceware.org

> Subject: Re: [PATCH v2] libc/time: Move internal newlib tz-structs to

> local.h

>

> On Oct 15 06:52, Torbjorn SVENSSON via Newlib wrote:

> > Ping!

>

> Due to vacation I only just checked now.  The patch breaks the Cygwin

> build.  Especially __tzinfo_type has to be exposed from a public header

> in newlib.

>

> So what about the attached patch instead?

>

>

> Corinna

>
Corinna Vinschen via Newlib Oct. 15, 2020, 5:11 p.m. | #5
On Oct 15 16:47, Torbjorn SVENSSON via Newlib wrote:
> Hello Corinna,

> 

> Thanks for the feedback and I hope that you had a nice vacation!

> 

> I haven't run tests with your patch applied, but just reading it should be fine.

> I see no reason why "sys/_tz_structs.h" would be included from

> "bits/stdc++.h", and it is this particular include chain that is

> causing the problems in the libstdc ++ test suite, so if it works for

> Cygwin, I'd say go for it!


Thanks for reviewing.  I'll push the patch in a minute.  I still
think we should rename the struct members as well, though.  There's
no good reason that we have a user of these structures outside
newlib/Cygwin.  But still, *iff* this file is included for whatever
dubious purpose, it might result in problems.

We have two ways to fix this:

- Either guard the definitions additionally with a preprocessor
  expression like this:

  #if defined (__INSIDE_CYGWIN__) || defined (_COMPILING_NEWLIB)
  [...]
  #endif

- or fix the names of the struct members and the newlib/Cygwin code
  using them.


Thoughts?  Jeff?


Corinna
Corinna Vinschen via Newlib Oct. 15, 2020, 5:15 p.m. | #6
On Oct 15 13:09, Jeff Johnston via Newlib wrote:
> I also like this solution, but again as I mentioned earlier, it technically

> requires a major release bump due to the removal of API from time.h and

> would be best

> to wait until we have something else warranting a major release bump.

> 

> That said, what do you think Corinna?


Oops, time warp!  Sorry, but I already pushed this.

From my POV, these *are* internal structs and functions, so,
technically, they are *not* official API and consequentially
nobody has the right to expect them in an official header.
For me the only problem is that the members are not underscored
and may produce problems.


Corinna


> 

> -- Jeff J.

> 

> On Thu, Oct 15, 2020 at 12:47 PM Torbjorn SVENSSON via Newlib <

> newlib@sourceware.org> wrote:

> 

> > Hello Corinna,

> >

> > Thanks for the feedback and I hope that you had a nice vacation!

> >

> > I haven't run tests with your patch applied, but just reading it should be

> > fine.

> > I see no reason why "sys/_tz_structs.h" would be included from

> > "bits/stdc++.h", and it is this particular include chain that is causing

> > the problems in the libstdc ++ test suite, so if it works for Cygwin, I'd

> > say go for it!

> >

> > Kind regards,

> > Torbjörn

> >

> > -----Original Message-----

> > From: Corinna Vinschen <vinschen@redhat.com>

> > Sent: den 15 oktober 2020 12:22

> > To: Torbjorn SVENSSON <torbjorn.svensson@st.com>

> > Cc: newlib@sourceware.org

> > Subject: Re: [PATCH v2] libc/time: Move internal newlib tz-structs to

> > local.h

> >

> > On Oct 15 06:52, Torbjorn SVENSSON via Newlib wrote:

> > > Ping!

> >

> > Due to vacation I only just checked now.  The patch breaks the Cygwin

> > build.  Especially __tzinfo_type has to be exposed from a public header

> > in newlib.

> >

> > So what about the attached patch instead?

> >

> >

> > Corinna

> >

Patch

diff --git a/newlib/libc/include/time.h b/newlib/libc/include/time.h
index 3031590b4..ed6cc70fe 100644
--- a/newlib/libc/include/time.h
+++ b/newlib/libc/include/time.h
@@ -102,26 +102,6 @@  void      tzset 	(void);
 #endif
 void      _tzset_r 	(struct _reent *);
 
-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);
-
 /* getdate functions */
 
 #ifdef HAVE_GETDATE
diff --git a/newlib/libc/sys/linux/include/time.h b/newlib/libc/sys/linux/include/time.h
index 5e61d2b65..917a35858 100644
--- a/newlib/libc/sys/linux/include/time.h
+++ b/newlib/libc/sys/linux/include/time.h
@@ -84,26 +84,6 @@  char      *strptime (const char *, const char *, struct tm *);
 void      tzset 	(void);
 void      _tzset_r 	(struct _reent *);
 
-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);
-
 /* getdate functions */
 
 #ifndef _REENT_ONLY
diff --git a/newlib/libc/sys/phoenix/include/time.h b/newlib/libc/sys/phoenix/include/time.h
index 3a9449c77..41fb137e4 100644
--- a/newlib/libc/sys/phoenix/include/time.h
+++ b/newlib/libc/sys/phoenix/include/time.h
@@ -40,23 +40,6 @@  extern char *_tzname[2];
 #define tzname	_tzname
 #endif
 
-typedef struct __tzrule_struct {
-	char ch;
-	int m;
-	int n;
-	int d;
-	int s;
-	time_t change;
-	long offset;
-} __tzrule_type;
-
-typedef struct __tzinfo_struct {
-	int __tznorth;
-	int __tzyear;
-	__tzrule_type __tzrule[2];
-} __tzinfo_type;
-
-__tzinfo_type *__gettzinfo();
 void tzset();
 
 clock_t clock();
diff --git a/newlib/libc/time/local.h b/newlib/libc/time/local.h
index dce51cda2..290e1aee5 100644
--- a/newlib/libc/time/local.h
+++ b/newlib/libc/time/local.h
@@ -38,3 +38,22 @@  void _tzset_unlocked (void);
 void __tz_lock (void);
 void __tz_unlock (void);
 
+typedef struct __tzrule_struct
+{
+  char ch;
+  int m; /* Month of year if ch=M */
+  int n; /* Week of month if ch=M */
+  int d; /* Day of week if ch=M, day of year if ch=J or ch=D */
+  int s; /* Time of day in seconds */
+  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);