Thanks for taking this up.
At Sat, 15 Oct 2022 20:59:00 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> On Thu, Oct 13, 2022 at 12:09:54PM -0700, Nathan Bossart wrote:
> > On Thu, Oct 13, 2022 at 12:37:39PM +0200, Alvaro Herrera wrote:
> >> The main reason is that it seems odd to have startpointTLI in the struct
> >> used in some places together with a file-global recvFileTLI which isn't.
> >> The way one is passed as argument and the other as part of a struct
> >> seemed too distracting. This should reduce the number of moving parts,
> >> ISTM.
> >
> > Makes sense. Do you think the struct should be file-global so that it
> > doesn't need to be provided as an argument to most of the static functions
> > in this file?
>
> Here's a different take. Instead of creating structs and altering function
> signatures, we can just make the wake-up times file-global, and we can skip
I'm fine with that. Rather it seems to me really simple.
> the changes related to reducing the number of calls to
> GetCurrentTimestamp() for now. This results in a far less invasive patch.
Sure.
> (I still think reducing the number of calls to GetCurrentTimestamp() is
> worthwhile, but I'm beginning to agree with Bharath that it should be
> handled separately.)
+1. We would gain lesser for the required effort.
> Thoughts?
Now that I see the fix for the implicit conversion:
L527: + nap = Max(0, (nextWakeup - now + 999) / 1000);
..
L545: + (int) Min(INT_MAX, nap),
I think limiting the naptime at use is confusing. Don't we place these
adjacently? Then we could change the nap to an integer. Or we can
just assert out for the nap time longer than INT_MAX (but this would
require another int64 variable. I belive we won't need such a long
nap, (or that is no longer a nap?)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center