Thread: Nitpick/question: Use of aliases for global variables in functions
I'm using an ongoing patch review to educate myself on parts of the codebase.
In src/backend/access/transam/xact.c, I'm noticing a code style inconsistency.
In some cases, a function will declare a variable of some struct pointer type, assign it to a globally declared struct, and then use it like this:
boolIsTransactionState(void){TransactionState s = CurrentTransactionState;
/** TRANS_DEFAULT and TRANS_ABORT are obviously unsafe states. However, we* also reject the startup/shutdown states TRANS_START, TRANS_COMMIT,* TRANS_PREPARE since it might be too soon or too late within those* transition states to do anything interesting. Hence, the only "valid"* state is TRANS_INPROGRESS.*/return (s->state == TRANS_INPROGRESS);}
And in other cases, the function will just reference the global directly
boolTransactionStartedDuringRecovery(void){return CurrentTransactionState->startedInRecovery;}
Clearly, this isn't a big deal. I'm willing to bet that the compiler looks at the variable declaration and thinks "welp, it's just going to end up in an address register anyway, may as well optimize it away" at even really low optimization levels, so I'd be surprised if there's even a difference in the machine code generated, though it might hinder inlining the function as a whole. Mostly I'm just noticing the inconsistent coding style.
Good points about the alias: I can see where the alias variable acts as a reminder of the data type, but even that is the type of a pointer to the struct, so we're still 1 level of indirection away instead of 2. I can also see where it might just be an artifact of getting stuff to fit within 80 chars.
Bad points about the alias: I can see where things might hinder function inlining, and might also hinder git grep searches for the global variable name (though the global in question is scoped to the file, so that's not a big concern).
Is this something worth standardizing, and if so, which style do we like better?
Corey Huinker <corey.huinker@gmail.com> writes:
> In src/backend/access/transam/xact.c, I'm noticing a code style
> inconsistency.
> [ to wit, local variable alias for CurrentTransactionState or not ]
> Is this something worth standardizing, and if so, which style do we like
> better?
I can't get excited about changing this. There may be historical
reasons for the differences, eg maybe at one time there was more
than one reference to the struct in IsTransactionState(). Or maybe
it was just different people doing it a bit differently. It's not
really a big enough difference to be a hindrance to readers, IMO
anyway, so I'd leave it alone.
regards, tom lane