diff mbox

[v3,3/7] y2038: alpha: Rename valid_timeval_to_timeval64 to valid_timeval32_to_timeval

Message ID 20200129125914.11221-3-lukma@denx.de
State New
Headers show

Commit Message

Lukasz Majewski Jan. 29, 2020, 12:59 p.m. UTC
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(-)

Comments

Adhemerval Zanella Feb. 4, 2020, 6:40 p.m. UTC | #1
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.
Lukasz Majewski Feb. 4, 2020, 10:50 p.m. UTC | #2
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
Adhemerval Zanella Feb. 5, 2020, 4:11 p.m. UTC | #3
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 mbox

Patch

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 };
 }