Thread: Small xlog.c cleanup
Hi,
Here's a teeny-tiny cleanup (and thus comprehensibility
fix) for xlog.c.
Matthew.
--- /home/matthew/xlog.c Tue Feb 27 17:12:24 2001
+++ xlog.c Tue Feb 27 17:16:04 2001
@@ -282,10 +282,6 @@
static XLogRecPtr ReadRecPtr;
static XLogRecPtr EndRecPtr;
-static int readFile = -1;
-static uint32 readId = 0;
-static uint32 readSeg = 0;
-static uint32 readOff = 0;
static char readBuf[BLCKSZ];
static XLogRecord *nextRecord = NULL;
@@ -1214,6 +1210,10 @@
bool nextmode = (RecPtr == NULL);
int emode = (nextmode) ? LOG : STOP;
bool noBlck = false;
+ static int readFile = -1;
+ static uint32 readId = 0;
+ static uint32 readSeg = 0;
+ static uint32 readOff = 0;
if (nextmode)
{
All source gets reformatted before release by src/tools/pgindent anyway.
> Hi,
>
> Here's a teeny-tiny cleanup (and thus comprehensibility
> fix) for xlog.c.
>
> Matthew.
>
>
> --- /home/matthew/xlog.c Tue Feb 27 17:12:24 2001
> +++ xlog.c Tue Feb 27 17:16:04 2001
> @@ -282,10 +282,6 @@
>
> static XLogRecPtr ReadRecPtr;
> static XLogRecPtr EndRecPtr;
> -static int readFile = -1;
> -static uint32 readId = 0;
> -static uint32 readSeg = 0;
> -static uint32 readOff = 0;
> static char readBuf[BLCKSZ];
> static XLogRecord *nextRecord = NULL;
>
> @@ -1214,6 +1210,10 @@
> bool nextmode = (RecPtr == NULL);
> int emode = (nextmode) ? LOG : STOP;
> bool noBlck = false;
> + static int readFile = -1;
> + static uint32 readId = 0;
> + static uint32 readSeg = 0;
> + static uint32 readOff = 0;
>
> if (nextmode)
> {
>
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
On Tue, 27 Feb 2001, Bruce Momjian wrote: > All source gets reformatted before release by src/tools/pgindent > anyway. This is more than just a formatting change -- it turns some static globals into static locals. Sorry, I should have been a little more verbose in my description. Matthew.
> On Tue, 27 Feb 2001, Bruce Momjian wrote:
>
> > All source gets reformatted before release by src/tools/pgindent
> > anyway.
>
> This is more than just a formatting change -- it turns
> some static globals into static locals.
>
> Sorry, I should have been a little more verbose in my
> description.
Can you point me to the change. I don't see it:
---------------------------------------------------------------------------
-static int readFile = -1;
-static uint32 readId = 0;
-static uint32 readSeg = 0;
-static uint32 readOff = 0;
static char readBuf[BLCKSZ];
static XLogRecord *nextRecord = NULL;
@@ -1214,6 +1210,10 @@
bool nextmode = (RecPtr == NULL);
int emode = (nextmode) ? LOG : STOP;
bool noBlck = false;
+ static int readFile = -1;
+ static uint32 readId = 0;
+ static uint32 readSeg = 0;
+ static uint32 readOff = 0;
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
On Tue, 27 Feb 2001, Bruce Momjian wrote: > > On Tue, 27 Feb 2001, Bruce Momjian wrote: > > > > > All source gets reformatted before release by src/tools/pgindent > > > anyway. > > > > This is more than just a formatting change -- it turns > > some static globals into static locals. > > > > Sorry, I should have been a little more verbose in my > > description. > > Can you point me to the change. I don't see it: > > --------------------------------------------------------------------------- > He's moved the "static globals" from around line 286 of src/backend/access/transam/xlog.c: > -static int readFile = -1; > -static uint32 readId = 0; > -static uint32 readSeg = 0; > -static uint32 readOff = 0; > static char readBuf[BLCKSZ]; > static XLogRecord *nextRecord = NULL; > to be static locals inside of function ReadRecord around line 1216 in the same file: > @@ -1214,6 +1210,10 @@ > bool nextmode = (RecPtr == NULL); > int emode = (nextmode) ? LOG : STOP; > bool noBlck = false; > + static int readFile = -1; > + static uint32 readId = 0; > + static uint32 readSeg = 0; > + static uint32 readOff = 0; > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 > Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy Systems Administrator @ hub.org primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
The Hermit Hacker <scrappy@hub.org> writes:
> He's moved the "static globals" from around line 286 of
> src/backend/access/transam/xlog.c:
> to be static locals inside of function ReadRecord around line 1216 in the
> same file:
This is not usual coding practice in Postgres, so far as I've noticed.
I prefer to avoid static locals because the fact that they *are* static
is easily missed. In particular it's way too easy to misread the
initialization as something that happens on every entry to the function,
rather than only once. So my opinion is that this change is bad style.
regards, tom lane
On Tue, 27 Feb 2001, Tom Lane wrote: > The Hermit Hacker <scrappy@hub.org> writes: > > He's moved the "static globals" from around line 286 of > > src/backend/access/transam/xlog.c: > > to be static locals inside of function ReadRecord around line 1216 in the > > same file: > > This is not usual coding practice in Postgres, so far as I've noticed. > > I prefer to avoid static locals because the fact that they *are* static > is easily missed. In particular it's way too easy to misread the > initialization as something that happens on every entry to the function, > rather than only once. So my opinion is that this change is bad style. Oops, don't take my response to Bruce as validation, only explanation of what he was accomplishing, which Bruce appeared to indicate he didn't understand ...
> On Tue, 27 Feb 2001, Tom Lane wrote: > > > The Hermit Hacker <scrappy@hub.org> writes: > > > He's moved the "static globals" from around line 286 of > > > src/backend/access/transam/xlog.c: > > > to be static locals inside of function ReadRecord around line 1216 in the > > > same file: > > > > This is not usual coding practice in Postgres, so far as I've noticed. > > > > I prefer to avoid static locals because the fact that they *are* static > > is easily missed. In particular it's way too easy to misread the > > initialization as something that happens on every entry to the function, > > rather than only once. So my opinion is that this change is bad style. > > Oops, don't take my response to Bruce as validation, only explanation of > what he was accomplishing, which Bruce appeared to indicate he didn't > understand ... Thanks. I was looking for 'static/no static' but now replace we was moving the defines, which makes them behave differently. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
On Tue, 27 Feb 2001, Tom Lane wrote: > > He's moved the "static globals" from around line 286 of > > src/backend/access/transam/xlog.c: > > to be static locals inside of function ReadRecord around line 1216 in the > > same file: > > This is not usual coding practice in Postgres, so far as I've noticed. > > I prefer to avoid static locals because the fact that they *are* > static is easily missed. In particular it's way too easy to misread > the initialization as something that happens on every entry to the > function, rather than only once. So my opinion is that this change is > bad style. Fair enough. I just found it even more confusing that they were defined almost 1KLOC from their only use. But functions of 300LOC tend to fail my taste guidelines too, so perhaps I should leave well alone :) Matthew.