Re: ksh INT32 type

[Philip Guenther]

On Fri, Sep 11, 2015 at 5:49 AM, Timo Buhrmester  wrote:
>> Can you clarify why you used int instead of int32_t?
> Considering that
>   - the comment around it said ``Find an integer type that is at least 32 
> bits''
>   - int may be less than 32 bits wide (C99 5.2.4.2.1)

While true in a standards sense, OpenBSD will never run on an arch
where int is less than 32 bits.  Are there people porting OpenBSD's
ksh to OSes where int can be 16 bits?  If so, *they* need to step up
and argue for making to code less readable.  Uglifying the code for a
hypothetical port is not our way.

Philip Guenther

Re: ksh INT32 type

[Nicholas Marriott]

I think all of these except perhaps Coproc_id would be better as plain
int not int32_t.

The typedefs could probably die completely (definitely Tflag anyway) but
separate diff.


On Wed, Sep 09, 2015 at 08:27:14PM -0400, Michael McConville wrote:
> I may be totally off base here, but:
> 
> INT32's comment suggests that the configure script checks that int is >=
> 32 bits. However, i don't think that script's around anymore, and ANSI
> specifies a minimum of only 16 bits.
> 
> The comment also says that INT32 can be 64 bits, but it's then used as
> Tflag, whose comment says it can't be > 32 bits.
> 
> Maybe we should just replace it with int32_t?
> 
> I also fixed a couple adjacent comment typos.
> 
> Index: jobs.c
> ===
> RCS file: /cvs/src/bin/ksh/jobs.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 jobs.c
> --- jobs.c18 Apr 2015 18:28:36 -  1.41
> +++ jobs.c10 Sep 2015 00:06:59 -
> @@ -71,7 +71,7 @@ struct job {
>   int status; /* exit status of last process */
>   pid_t   pgrp;   /* process group of job */
>   pid_t   ppid;   /* pid of process that forked job */
> - INT32   age;/* number of jobs started */
> + int32_t age;/* number of jobs started */
>   struct timeval systime; /* system time used by job */
>   struct timeval usrtime; /* user time used by job */
>   Proc*proc_list; /* process list */
> @@ -111,7 +111,7 @@ static Job*async_job;
>  static pid_t async_pid;
>  
>  static int   nzombie;/* # of zombies owned by this process */
> -INT32njobs;  /* # of jobs started */
> +int32_t  njobs;  /* # of jobs started */
>  static int   child_max;  /* CHILD_MAX */
>  
>  
> Index: sh.h
> ===
> RCS file: /cvs/src/bin/ksh/sh.h,v
> retrieving revision 1.33
> diff -u -p -r1.33 sh.h
> --- sh.h  18 Dec 2013 13:53:12 -  1.33
> +++ sh.h  10 Sep 2015 00:06:59 -
> @@ -28,12 +28,6 @@
>  
>  #include 
>  
> -/* Find a integer type that is at least 32 bits (or die) - SIZEOF_* defined
> - * by autoconf (assumes an 8 bit byte, but I'm not concerned).
> - * NOTE: INT32 may end up being more than 32 bits.
> - */
> -# define INT32   int
> -
>  /* end of common headers */
>  
>  /* some useful #defines */
> @@ -52,8 +46,8 @@
>  #define  sizeofN(type, n) (sizeof(type) * (n))
>  #define  BIT(i)  (1<<(i))/* define bit in flag */
>  
> -/* Table flag type - needs > 16 and < 32 bits */
> -typedef INT32 Tflag;
> +/* Table flag type - needs >= 16 and <= 32 bits */
> +typedef int32_t Tflag;
>  
>  #define  NUFILE  32  /* Number of user-accessible files */
>  #define  FDBASE  10  /* First file usable by Shell */
> @@ -353,7 +347,7 @@ EXTERN Getopt user_opt;   /* parsing stat
>  
>  /* This for co-processes */
>  
> -typedef INT32 Coproc_id; /* something that won't (realisticly) wrap */
> +typedef int32_t Coproc_id; /* something that won't (realistically) wrap */
>  struct coproc {
>   int read;   /* pipe from co-process's stdout */
>   int readw;  /* other side of read (saved temporarily) */
> 

Re: ksh INT32 type

[Nicholas Marriott]

It would be more helpful to post the diff again rather than a link
buried in another thread.

Any oks for this?


Index: jobs.c
===
RCS file: /cvs/src/bin/ksh/jobs.c,v
retrieving revision 1.41
diff -u -p -r1.41 jobs.c
--- jobs.c  18 Apr 2015 18:28:36 -  1.41
+++ jobs.c  10 Sep 2015 11:45:02 -
@@ -71,7 +71,7 @@ struct job {
int status; /* exit status of last process */
pid_t   pgrp;   /* process group of job */
pid_t   ppid;   /* pid of process that forked job */
-   INT32   age;/* number of jobs started */
+   int age;/* number of jobs started */
struct timeval systime; /* system time used by job */
struct timeval usrtime; /* user time used by job */
Proc*proc_list; /* process list */
@@ -111,7 +111,7 @@ static Job  *async_job;
 static pid_t   async_pid;
 
 static int nzombie;/* # of zombies owned by this process */
-INT32  njobs;  /* # of jobs started */
+intnjobs;  /* # of jobs started */
 static int child_max;  /* CHILD_MAX */
 
 
Index: sh.h
===
RCS file: /cvs/src/bin/ksh/sh.h,v
retrieving revision 1.33
diff -u -p -r1.33 sh.h
--- sh.h18 Dec 2013 13:53:12 -  1.33
+++ sh.h10 Sep 2015 11:45:02 -
@@ -28,12 +28,6 @@
 
 #include 
 
-/* Find a integer type that is at least 32 bits (or die) - SIZEOF_* defined
- * by autoconf (assumes an 8 bit byte, but I'm not concerned).
- * NOTE: INT32 may end up being more than 32 bits.
- */
-# define INT32 int
-
 /* end of common headers */
 
 /* some useful #defines */
@@ -53,7 +47,7 @@
 #defineBIT(i)  (1<<(i))/* define bit in flag */
 
 /* Table flag type - needs > 16 and < 32 bits */
-typedef INT32 Tflag;
+typedef int Tflag;
 
 #defineNUFILE  32  /* Number of user-accessible files */
 #defineFDBASE  10  /* First file usable by Shell */
@@ -353,7 +347,7 @@ EXTERN Getopt user_opt; /* parsing stat
 
 /* This for co-processes */
 
-typedef INT32 Coproc_id; /* something that won't (realisticly) wrap */
+typedef int Coproc_id; /* something that won't (realistically) wrap */
 struct coproc {
int read;   /* pipe from co-process's stdout */
int readw;  /* other side of read (saved temporarily) */



On Thu, Sep 10, 2015 at 01:13:24PM +0200, Martijn van Duren wrote:
> I already sent this diff on September 1st.[1]
> Pointed out in private to and ok by nicm@
> 
> [1]http://marc.info/?l=openbsd-tech=144112883814618=2
> 
> On 09/10/15 11:05, Nicholas Marriott wrote:
> >I think all of these except perhaps Coproc_id would be better as plain
> >int not int32_t.
> >
> >The typedefs could probably die completely (definitely Tflag anyway) but
> >separate diff.
> >
> >
> >On Wed, Sep 09, 2015 at 08:27:14PM -0400, Michael McConville wrote:
> >>I may be totally off base here, but:
> >>
> >>INT32's comment suggests that the configure script checks that int is >=
> >>32 bits. However, i don't think that script's around anymore, and ANSI
> >>specifies a minimum of only 16 bits.
> >>
> >>The comment also says that INT32 can be 64 bits, but it's then used as
> >>Tflag, whose comment says it can't be > 32 bits.
> >>
> >>Maybe we should just replace it with int32_t?
> >>
> >>I also fixed a couple adjacent comment typos.
> >>
> >>Index: jobs.c
> >>===
> >>RCS file: /cvs/src/bin/ksh/jobs.c,v
> >>retrieving revision 1.41
> >>diff -u -p -r1.41 jobs.c
> >>--- jobs.c  18 Apr 2015 18:28:36 -  1.41
> >>+++ jobs.c  10 Sep 2015 00:06:59 -
> >>@@ -71,7 +71,7 @@ struct job {
> >>int status; /* exit status of last process */
> >>pid_t   pgrp;   /* process group of job */
> >>pid_t   ppid;   /* pid of process that forked job */
> >>-   INT32   age;/* number of jobs started */
> >>+   int32_t age;/* number of jobs started */
> >>struct timeval systime; /* system time used by job */
> >>struct timeval usrtime; /* user time used by job */
> >>Proc*proc_list; /* process list */
> >>@@ -111,7 +111,7 @@ static Job  *async_job;
> >>  static pid_t  async_pid;
> >>
> >>  static intnzombie;/* # of zombies owned by this 
> >> process */
> >>-INT32  njobs;  /* # of jobs started */
> >>+int32_tnjobs;  /* # of jobs started */
> >>  static intchild_max;  /* CHILD_MAX */
> >>
> >>
> >>Index: sh.h
> >>===
> >>RCS file: /cvs/src/bin/ksh/sh.h,v
> >>retrieving revision 1.33
> >>diff -u -p 

Re: ksh INT32 type

[Todd C. Miller]

On Thu, 10 Sep 2015 12:51:37 +0100, Nicholas Marriott wrote:

> It would be more helpful to post the diff again rather than a link
> buried in another thread.
> 
> Any oks for this?

OK millert@

 - todd

Re: ksh INT32 type

[Martijn van Duren]

On 09/10/15 15:56, Michael McConville wrote:

Martijn van Duren wrote:

I already sent this diff on September 1st.[1] Pointed out in private
to and ok by nicm@

[1]http://marc.info/?l=openbsd-tech=144112883814618=2


Ah, awkward. I hadn't seen this.

Can you clarify why you used int instead of int32_t?

Because that's what the original define used, so the compiled code would 
be the same.

Re: ksh INT32 type

[Michael McConville]

Martijn van Duren wrote:
> I already sent this diff on September 1st.[1] Pointed out in private
> to and ok by nicm@
> 
> [1]http://marc.info/?l=openbsd-tech=144112883814618=2

Ah, awkward. I hadn't seen this.

Can you clarify why you used int instead of int32_t?

Re: ksh INT32 type

[Ted Unangst]

> - * NOTE: INT32 may end up being more than 32 bits.

>  /* Table flag type - needs > 16 and < 32 bits */
> -typedef INT32 Tflag;

awkward...

Re: ksh INT32 type

[Timo Buhrmester]

> Can you clarify why you used int instead of int32_t?
Considering that
  - the comment around it said ``Find an integer type that is at least 32 bits''
  - int may be less than 32 bits wide (C99 5.2.4.2.1)
  - int32_t is not guaranteed to exist (C99 7.18.1.1p3)
The most appropriate type would be int_least32_t, no? (required to exist (C99 
7.18.1.2p3))

(That said, I'm not sure if there's a port for which this could be a problem, 
so if not, feel free to disregard)

--
Timo Buhrmester