Re: [PATCH v4 00/12] nptl: Implement POSIX-proposed _clock variants of existing _timed functions

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 27/06/2019 14:15, Mike Crowe wrote:
> On Wednesday 26 June 2019 at 15:41:24 -0300, Adhemerval Zanella wrote:
>> On 25/06/2019 19:13, Adhemerval Zanella wrote:
>>>
>>>
>>> On 18/06/2019 13:33, Mike Crowe wrote:
>>>> My attempts[1] to add a variant of pthread_cond_timedwait that would accept
>>>> a clockid_t parameter led me to propose[2] to The Austin Group the addition
>>>> of an entire family of functions that accept a clockid_t parameter to
>>>> indicate the clock that the timespec absolute timeout parameter should be
>>>> measured against. They responded positively to the request but an
>>>> implementation is required before the proposal can proceed.
>>>>
>>>> This patch series is the second version of the first part of that
>>>> implementation in glibc, it contains implementations and tests for
>>>> four new functions:
>>>>
>>>> int pthread_cond_clockwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
>>>>                            clockid_t clock, const struct timespec *abstime)
>>>>
>>>> int pthread_rwlock_clockrdlock(pthread_rwlock_t *rwlock, clockid_t clock,
>>>>                                const struct timespec *abstime)
>>>>
>>>> int pthread_rwlock_clockwrlock(pthread_rwlock_t *rwlock, clockid_t clock,
>>>>                                const struct timespec *abstime)
>>>>
>>>> int sem_clockwait(sem_t *restrict, clockid_t clock_id, const struct
>>>>                   timespec *restrict)
>>>>
>>>> int pthread_mutex_clocklock (pthread_mutex_t *mutex,
>>>> 			     clockid_t clockid,
>>>> 			     const struct timespec *abstime)
>>>
>>> Hi Mike, 
>>>
>>> I have created a personal branch [1] with remaining patches with the small
>>> fixes I pointed on the review.  It just to make it simpler you give me ack
>>> and avoid resending all the patch all over again.
>>>
>>> Florian, I am using the original naming scheme as indicated above. I think
>>> we should follow what is expected from standard once we have an actual 
>>> implementation.
>>>
>>> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/master-posix_clock
>>>
>>
>> Hi Mike, I have updated the branch with some other style fixes (mostly
>> regarding missing space before period).
> 
> Hi Adhemerval,
> 
> I've looked at the branch. It looks like you've used a tool to regenerate
> the ChangeLog from the commit messages. This means that some of my more
> detailed descriptions have gone missing (which is presumably fine.)
> However, it means that a mistake in the commit message for
> 
>  nptl: Add POSIX-proposed pthread_rwlock_clockrdlock & pthread_rwlock_clockwrlock
> 
> is now in the ChangeLog too. Neither should contain the line:

I think I have corrected it and added the missing descriptions you have
added on your original submission.

> 
> I'll never get the hang of "two spaces after a period". Thanks for fixing
> them up. I spotted that you've missed one in
> sysdeps/unix/sysv/linux/lowlevellock-futex.h though:
> 
>  CLOCK_REALTIME timeouts for FUTEX_WAIT_BITSET. We could attempt to

Fixed.

> 
> It looks like you've also reformatted many of the comments with a longer
> line length. I used the Emacs default of 70 characters. Should I have used
> 79 or 80?

According to our syle and conventions wiki you should use 79-column lines [1].

> 
> Thanks for doing all this. It's looking quite tight to get this in for
> v2.30 though. Is there anything else you're waiting for me to do?

We need to get consensus on the naming scheme for the symbols itself. 
The personal branches uses the expected names I think we should go
for it. Florian seems to have doubts about it yet.

> 
> Mike.
> 

[1] https://sourceware.org/glibc/wiki/Style_and_Conventions