| Message ID | 20200129125914.11221-3-lukma@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/7] y2038: Define __suseconds64_t type to be used with struct __timeval64 | expand |
On 29/01/2020 09:59, Lukasz Majewski wrote: > Without this patch the naming convention for functions to convert > struct timeval32 to struct timeval (which supports 64 bit time on Alpha) was > a bit misleading. The name 'valid_timeval_to_timeval64' suggest conversion > of struct timeval to struct __timeval64 (as in ./include/time.h). > > As on alpha the struct timeval supports 64 bit time it seems more readable > to emphasis struct timeval32 in the conversion function name. > > Hence the helper function naming change to 'valid_timeval32_to_timeval'. > > Build tests: > ./src/scripts/build-many-glibcs.py glibcs > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Usually I don't see much gain in such changes. It the current name clashing with a new proposed internal symbol? Anyway, it seems fine though. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > --- > Changes for v3: > - None > > Changes for v2: > - None > --- > sysdeps/unix/sysv/linux/alpha/osf_adjtime.c | 4 ++-- > sysdeps/unix/sysv/linux/alpha/osf_setitimer.c | 4 ++-- > sysdeps/unix/sysv/linux/alpha/osf_utimes.c | 4 ++-- > sysdeps/unix/sysv/linux/alpha/tv32-compat.h | 2 +- > 4 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c > index cd864686f6..5ac72e252f 100644 > --- a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c > +++ b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c > @@ -57,7 +57,7 @@ int > attribute_compat_text_section > __adjtime_tv32 (const struct timeval32 *itv, struct timeval32 *otv) > { > - struct timeval itv64 = valid_timeval_to_timeval64 (*itv); > + struct timeval itv64 = valid_timeval32_to_timeval (*itv); > struct timeval otv64; > > if (__adjtime (&itv64, &otv64) == -1) > @@ -91,7 +91,7 @@ __adjtimex_tv32 (struct timex32 *tx) > tx64.calcnt = tx->calcnt; > tx64.errcnt = tx->errcnt; > tx64.stbcnt = tx->stbcnt; > - tx64.time = valid_timeval_to_timeval64 (tx->time); > + tx64.time = valid_timeval32_to_timeval (tx->time); > > int status = __adjtimex (&tx64); > if (status < 0) Ok. > diff --git a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c > index 418efbf546..3935d1cfb5 100644 > --- a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c > +++ b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c > @@ -30,9 +30,9 @@ __setitimer_tv32 (int which, const struct itimerval32 *restrict new_value, > { > struct itimerval new_value_64; > new_value_64.it_interval > - = valid_timeval_to_timeval64 (new_value->it_interval); > + = valid_timeval32_to_timeval (new_value->it_interval); > new_value_64.it_value > - = valid_timeval_to_timeval64 (new_value->it_value); > + = valid_timeval32_to_timeval (new_value->it_value); > > if (old_value == NULL) > return __setitimer (which, &new_value_64, NULL); Ok. > diff --git a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c > index 423c2a8ef2..6c3fad0132 100644 > --- a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c > +++ b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c > @@ -28,8 +28,8 @@ attribute_compat_text_section > __utimes_tv32 (const char *filename, const struct timeval32 times32[2]) > { > struct timeval times[2]; > - times[0] = valid_timeval_to_timeval64 (times32[0]); > - times[1] = valid_timeval_to_timeval64 (times32[1]); > + times[0] = valid_timeval32_to_timeval (times32[0]); > + times[1] = valid_timeval32_to_timeval (times32[1]); > return __utimes (filename, times); > } > Ok. > diff --git a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h b/sysdeps/unix/sysv/linux/alpha/tv32-compat.h > index 6076d2ec05..7169909259 100644 > --- a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h > +++ b/sysdeps/unix/sysv/linux/alpha/tv32-compat.h > @@ -70,7 +70,7 @@ struct rusage32 > overflow, they write { INT32_MAX, TV_USEC_MAX } to the output. */ > > static inline struct timeval > -valid_timeval_to_timeval64 (const struct timeval32 tv) > +valid_timeval32_to_timeval (const struct timeval32 tv) > { > return (struct timeval) { tv.tv_sec, tv.tv_usec }; > } > Ok.
Hi Adhemerval, > On 29/01/2020 09:59, Lukasz Majewski wrote: > > Without this patch the naming convention for functions to convert > > struct timeval32 to struct timeval (which supports 64 bit time on > > Alpha) was a bit misleading. The name 'valid_timeval_to_timeval64' > > suggest conversion of struct timeval to struct __timeval64 (as in > > ./include/time.h). > > > > As on alpha the struct timeval supports 64 bit time it seems more > > readable to emphasis struct timeval32 in the conversion function > > name. > > > > Hence the helper function naming change to > > 'valid_timeval32_to_timeval'. > > > > Build tests: > > ./src/scripts/build-many-glibcs.py glibcs > > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Usually I don't see much gain in such changes. It the current name > clashing with a new proposed internal symbol? The problem here is that the name is a bit misleading, as alpha's struct timeval has 64 bit tv.sec (alpha generally supports 64 bit time). The rename here is to emphasis that we do convert "natural" (for alpha) struct timeval to struct timeval32 (which is needed when passing the pointer to syscalls). In that way the 'valid_timeval_to_timeval64' can follow the pattern, which we do have now in ./include/time.h. (it is just to make the naming convention more consistent) Just to point out - Alistair in his patch set (prepared on top of this one): https://patchwork.ozlabs.org/patch/1232967/ took one step further and added "alpha_" prefix to those names (converted in this patch). > > Anyway, it seems fine though. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > > > > --- > > Changes for v3: > > - None > > > > Changes for v2: > > - None > > --- > > sysdeps/unix/sysv/linux/alpha/osf_adjtime.c | 4 ++-- > > sysdeps/unix/sysv/linux/alpha/osf_setitimer.c | 4 ++-- > > sysdeps/unix/sysv/linux/alpha/osf_utimes.c | 4 ++-- > > sysdeps/unix/sysv/linux/alpha/tv32-compat.h | 2 +- > > 4 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c > > b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c index > > cd864686f6..5ac72e252f 100644 --- > > a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c +++ > > b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c @@ -57,7 +57,7 @@ int > > attribute_compat_text_section > > __adjtime_tv32 (const struct timeval32 *itv, struct timeval32 *otv) > > { > > - struct timeval itv64 = valid_timeval_to_timeval64 (*itv); > > + struct timeval itv64 = valid_timeval32_to_timeval (*itv); > > struct timeval otv64; > > > > if (__adjtime (&itv64, &otv64) == -1) > > @@ -91,7 +91,7 @@ __adjtimex_tv32 (struct timex32 *tx) > > tx64.calcnt = tx->calcnt; > > tx64.errcnt = tx->errcnt; > > tx64.stbcnt = tx->stbcnt; > > - tx64.time = valid_timeval_to_timeval64 (tx->time); > > + tx64.time = valid_timeval32_to_timeval (tx->time); > > > > int status = __adjtimex (&tx64); > > if (status < 0) > > Ok. > > > diff --git a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c > > b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c index > > 418efbf546..3935d1cfb5 100644 --- > > a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c +++ > > b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c @@ -30,9 +30,9 @@ > > __setitimer_tv32 (int which, const struct itimerval32 *restrict > > new_value, { struct itimerval new_value_64; > > new_value_64.it_interval > > - = valid_timeval_to_timeval64 (new_value->it_interval); > > + = valid_timeval32_to_timeval (new_value->it_interval); > > new_value_64.it_value > > - = valid_timeval_to_timeval64 (new_value->it_value); > > + = valid_timeval32_to_timeval (new_value->it_value); > > > > if (old_value == NULL) > > return __setitimer (which, &new_value_64, NULL); > > Ok. > > > diff --git a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c > > b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c index > > 423c2a8ef2..6c3fad0132 100644 --- > > a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c +++ > > b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c @@ -28,8 +28,8 @@ > > attribute_compat_text_section __utimes_tv32 (const char *filename, > > const struct timeval32 times32[2]) { > > struct timeval times[2]; > > - times[0] = valid_timeval_to_timeval64 (times32[0]); > > - times[1] = valid_timeval_to_timeval64 (times32[1]); > > + times[0] = valid_timeval32_to_timeval (times32[0]); > > + times[1] = valid_timeval32_to_timeval (times32[1]); > > return __utimes (filename, times); > > } > > > > Ok. > > > diff --git a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h > > b/sysdeps/unix/sysv/linux/alpha/tv32-compat.h index > > 6076d2ec05..7169909259 100644 --- > > a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h +++ > > b/sysdeps/unix/sysv/linux/alpha/tv32-compat.h @@ -70,7 +70,7 @@ > > struct rusage32 overflow, they write { INT32_MAX, TV_USEC_MAX } to > > the output. */ > > static inline struct timeval > > -valid_timeval_to_timeval64 (const struct timeval32 tv) > > +valid_timeval32_to_timeval (const struct timeval32 tv) > > { > > return (struct timeval) { tv.tv_sec, tv.tv_usec }; > > } > > > > Ok. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 04/02/2020 19:50, Lukasz Majewski wrote: > Hi Adhemerval, > >> On 29/01/2020 09:59, Lukasz Majewski wrote: >>> Without this patch the naming convention for functions to convert >>> struct timeval32 to struct timeval (which supports 64 bit time on >>> Alpha) was a bit misleading. The name 'valid_timeval_to_timeval64' >>> suggest conversion of struct timeval to struct __timeval64 (as in >>> ./include/time.h). >>> >>> As on alpha the struct timeval supports 64 bit time it seems more >>> readable to emphasis struct timeval32 in the conversion function >>> name. >>> >>> Hence the helper function naming change to >>> 'valid_timeval32_to_timeval'. >>> >>> Build tests: >>> ./src/scripts/build-many-glibcs.py glibcs >>> >>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >> >> Usually I don't see much gain in such changes. It the current name >> clashing with a new proposed internal symbol? > > The problem here is that the name is a bit misleading, as alpha's > struct timeval has 64 bit tv.sec (alpha generally supports 64 bit time). > > The rename here is to emphasis that we do convert "natural" (for alpha) > struct timeval to struct timeval32 (which is needed when passing the > pointer to syscalls). > > In that way the 'valid_timeval_to_timeval64' can follow the pattern, > which we do have now in ./include/time.h. > > (it is just to make the naming convention more consistent) I understand it and Iam not really against it in fact, it is usually I see refactoring more profitable when it simplifies the resulting code by either removing redundancy or adhering to newer code practices.
diff --git a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c index cd864686f6..5ac72e252f 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c @@ -57,7 +57,7 @@ int attribute_compat_text_section __adjtime_tv32 (const struct timeval32 *itv, struct timeval32 *otv) { - struct timeval itv64 = valid_timeval_to_timeval64 (*itv); + struct timeval itv64 = valid_timeval32_to_timeval (*itv); struct timeval otv64; if (__adjtime (&itv64, &otv64) == -1) @@ -91,7 +91,7 @@ __adjtimex_tv32 (struct timex32 *tx) tx64.calcnt = tx->calcnt; tx64.errcnt = tx->errcnt; tx64.stbcnt = tx->stbcnt; - tx64.time = valid_timeval_to_timeval64 (tx->time); + tx64.time = valid_timeval32_to_timeval (tx->time); int status = __adjtimex (&tx64); if (status < 0) diff --git a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c index 418efbf546..3935d1cfb5 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c @@ -30,9 +30,9 @@ __setitimer_tv32 (int which, const struct itimerval32 *restrict new_value, { struct itimerval new_value_64; new_value_64.it_interval - = valid_timeval_to_timeval64 (new_value->it_interval); + = valid_timeval32_to_timeval (new_value->it_interval); new_value_64.it_value - = valid_timeval_to_timeval64 (new_value->it_value); + = valid_timeval32_to_timeval (new_value->it_value); if (old_value == NULL) return __setitimer (which, &new_value_64, NULL); diff --git a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c index 423c2a8ef2..6c3fad0132 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c @@ -28,8 +28,8 @@ attribute_compat_text_section __utimes_tv32 (const char *filename, const struct timeval32 times32[2]) { struct timeval times[2]; - times[0] = valid_timeval_to_timeval64 (times32[0]); - times[1] = valid_timeval_to_timeval64 (times32[1]); + times[0] = valid_timeval32_to_timeval (times32[0]); + times[1] = valid_timeval32_to_timeval (times32[1]); return __utimes (filename, times); } diff --git a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h b/sysdeps/unix/sysv/linux/alpha/tv32-compat.h index 6076d2ec05..7169909259 100644 --- a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h +++ b/sysdeps/unix/sysv/linux/alpha/tv32-compat.h @@ -70,7 +70,7 @@ struct rusage32 overflow, they write { INT32_MAX, TV_USEC_MAX } to the output. */ static inline struct timeval -valid_timeval_to_timeval64 (const struct timeval32 tv) +valid_timeval32_to_timeval (const struct timeval32 tv) { return (struct timeval) { tv.tv_sec, tv.tv_usec }; }
Without this patch the naming convention for functions to convert struct timeval32 to struct timeval (which supports 64 bit time on Alpha) was a bit misleading. The name 'valid_timeval_to_timeval64' suggest conversion of struct timeval to struct __timeval64 (as in ./include/time.h). As on alpha the struct timeval supports 64 bit time it seems more readable to emphasis struct timeval32 in the conversion function name. Hence the helper function naming change to 'valid_timeval32_to_timeval'. Build tests: ./src/scripts/build-many-glibcs.py glibcs Reviewed-by: Alistair Francis <alistair.francis@wdc.com> --- Changes for v3: - None Changes for v2: - None --- sysdeps/unix/sysv/linux/alpha/osf_adjtime.c | 4 ++-- sysdeps/unix/sysv/linux/alpha/osf_setitimer.c | 4 ++-- sysdeps/unix/sysv/linux/alpha/osf_utimes.c | 4 ++-- sysdeps/unix/sysv/linux/alpha/tv32-compat.h | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-)