RE: [Proposal] Level4 Warnings show many shadow vars

De: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Enviado: segunda-feira, 9 de dezembro de 2019 03:40
>The file-scoped variable is needed to be process-persistent in any
>way. If we inhibit them, the upper-modules need to create the
>persistent area instead, for example, by calling XLogInitGlobals() or
>such, which makes things messier. Globality doens't necessarily mean
>evil and there're reasons for -Wall doesn't warn the case. I believe
>we, and especially committers are not who should be kept away from
>knives for the reason that knives generally have a possibility to
>injure someone.
Which harms the reusability of the code anyway.

>I might be too accustomed there, but the functions that define
>overriding locals don't modify the local variables and only the
>functions that don't override the globals modifies the glboals. I see
>no significant confusion here. By the way changes like "conf_file" ->
>"conffile" seems really useless as a fix patch.
Well i was trying to fix everything.

>As Robert said, they are harmless as far as we notice. Actual bugs
>caused by variable overriding would be welcomed to fix. I don't
>believe "lead to better performance and reduction (of code?)" without
>an evidence since modern compilers I think are not so stupid. Even if
>any, performance change in such extent doesn't support the proposal to
>remove variable overrides that way.

It's clear to me now that unless "the thing" is clearly a bug, don't touch it.
I love C, so for me it's very hard to resist getting stupid things like:
foo ()
{
int i, n;
for (i-0; i < n; i ++);
{
int i;
for (i=0; i < n; i ++);
}
{
int i;
for (i=0; i < n; i ++);
}
return;

I don't know how you can do it.

Of course, there are cases and cases, let's look at the example of multixact.c
diff --git a / src / backend / access / transam / multixact.c b / src / backend / access / transam / multixact.c
index 7b2448e05b..6364014fb3 100644
--- a / src / backend / access / transam / multixact.c
+++ b / src / backend / access / transam / multixact.c
@@ -1589.10 +1589.10 @@ mXactCachePut (MultiXactId multi, int nmembers, MultiXactMember * members)
qsort (entry-> members, nmembers, sizeof (MultiXactMember), mxactMemberComparator);

dlist_push_head (& MXactCache, & entry-> node);
+ pfree (entry); // <- is it really necessary?
if (MXactCacheMembers ++> = MAX_CACHE_ENTRIES)
{
dlist_node * node;
- mXactCacheEnt * entry;

node = dlist_tail_node (& MXactCache);
dlist_delete (node);

I still can't decide if it's a bug or not.

If it is a bug the correct function here is pfree or what is the equivalent function to free memory?

>Anyway I strongly object to the name 'pRedoRecPtr', which suggests as
>if it is a C-pointer to some variable. (And I believe we use Hungarian
>notation only if we don't have a better way...) LatestRedoRecPtr
>looks better to me.
I don't have enough information to decide if the lastest is the proper name, so I tried to change the nomenclature as little as possible.

I'll submit a patch sample, which depending on the answer, will give me if it's worth it or not, keep working on it.

regards,
Ranier Vilela