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:
bool
IsTransactionState(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
bool
TransactionStartedDuringRecovery(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?
