Re: [HACKERS] Some cleanups/enhancements

jeroenv@design.nl, can you send in patches for these?

>
> > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So
> > far no problems, however I noted some cleanups / enhancements which I
> > would like to do. Before I send you a bunch of patches I thought I'll
> > tell you what I'm planning to do. Please comment on my list and indicate
> > whether I should go ahead.
> >
> > - Fix all Makefiles so 'make dep' and 'make depend' work
> > - Fix all Makefiles so 'make clean' throws away the depend file
> > - Some other Makefile cleanups
>
> These all sound good. If there is a possibility of large breakage, wait
> until after v6.3.
>
> > - gcc 2.8.0 issues some additional warnings which are very easy to fix:
> >   - register i --> register int i
>
> I'm sure there will be differing opinions :-/, but imho all register
> declarations should be removed. Modern compilers do a good job of
> optimization, and register declarations can get in the way by forcing the
> compiler to burn a register to accomodate the declared item.
>
> >   - Ambiguous else --> add braces:
> >     if (cond1)
> >       if (cond2)
> >         ...
> >       else
> >         ...
>
> Sure.
>
> > - Add a template for linux-elf-586 with (optimized) code for a Pentium
> > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro).
> > Why not use template names similar to the output of config.guess (maybe
> > with some symbolic links)?
>
> Does gcc 2.7.x support the -mpentium and -mpentiumpro switches? If not,
> then the template should be more explicit in name (e.g.
> "linux-elf-586-gcc2.8") or we should update the FAQ or include comments in
> linux-elf with some suggestions. It was only in the last release or two
> that the -m486 was added, and I worried about causing trouble for _all_ of
> those 386 users :)
>
> > - Shouldn't same_tuple in executor/nodeGroup.c use the equality operator
> > for the type concerned to test for equality of the attributes rather
> > than print them to a buffer and use strcmp? Shouldn't the pointers for
> > these functions be looked up once in ExecInitGroup and stored somewhere?
> > Shouldn't this function go to heaptuple.c and be renamed heap_sametuple?
>
> Yes, I would think so. The only downside to this is that, since two items
> which fail the equality test may look identical when formatted (e.g.
> floating point numbers with the lsb differing) the results may look a bit
> funny and be difficult to track down. Still, I think this is the right way
> to go...
>
> > - Lump heaptuple.c and heapvalid.c together
> > - I also saw quite some #ifdef NOT_USED and other similar stuff. I don't
> > want to touch these now, but shouldn't some of these be removed soon?
>
> Only when the module is completely understood. So, don't remove blindly,
> but if it is clear that it is obsolete code which is not providing hints on
> what should be done in the future, then it is OK to remove.
>
> > - Add a pg_version function that returns a string like 'PostgreSQL 6.3'
> > to indicate the version of PostgreSQL a user is using (with 'select
> > pg_version()'). Might be handy to include in the bug reports.
>
> Good idea.
>
> Some or all of these changes might not be appropriate for v6.3, since we
> are in beta testing and since they do not affect the current functionality.
> For those cases, how about submitting patches based on the final v6.3
> release?
>
>                                                               - Tom
>
>
>


--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)