Re: Signed vs Unsigned (take 2) (src/backend/storage/ipc/procarray.c)

Em qui., 15 de jul. de 2021 às 09:45, David Rowley <dgrowleyml(at)gmail(dot)com>
escreveu:

> On Thu, 15 Jul 2021 at 23:38, Aleksander Alekseev
> <aleksander(at)timescale(dot)com> wrote:
> > I'm updating the status to "Ready for Committer".
>
> I think that might be a bit premature. I can't quite see how changing
> the pids List to a const List makes any sense, especially when the
> code goes and calls lappend_int() on it to assign it some different
> value.
>
> There are also problems in BackendPidGetProcWithLock around consts.
>
> Much of this patch kinda feels like another one of those "I've got a
> fancy new static analyzer" patches. Unfortunately, it just introduces
> a bunch of compiler warnings as a result of the changes it makes.
>
> I'd suggest splitting each portion of the patch out into parts related
> to what it aims to achieve. For example, it looks like there's some
> renaming going on to remove a local variable from shadowing a function
> parameter. Yet the patch is claiming performance improvements. I
> don't see how that part relates to performance. The changes to
> ProcArrayClearTransaction() seem also unrelated to performance.
>
> I'm not sure what the point of changing things like for (int i =0...
> to move the variable declaration somewhere else is about. That just
> seems like needless stylistic changes that achieve nothing but more
> headaches for committers doing backpatching work.
>
> I'd say if this patch wants to be taken seriously it better decide
> what it's purpose is, because to me it looks just like a jumble of
> random changes that have no clear purpose.
>
> I'm going to set this back to waiting on author.
>
I understood.
I will try to address all concerns in the new version.

regards,
Ranier Vilela