Thread: pgindent issue with EXEC_BACKEND-only typedefs
Hi, It seems pgindent is not considering EXEC_BACKEND typedefs. For example, static void restore_backend_variables(BackendParameters * param, Port *port); BackendParameters is not considered a typedef. Not sure how serious an issue this is ... I just noticed and thought I would mention it. -- Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4 "El miedo atento y previsor es la madre de la seguridad" (E. Burke)
Alvaro Herrera wrote: > Hi, > > It seems pgindent is not considering EXEC_BACKEND typedefs. For > example, > > static void restore_backend_variables(BackendParameters * param, Port *port); > > BackendParameters is not considered a typedef. > > Not sure how serious an issue this is ... I just noticed and thought I > would mention it. Yep. The cause is that find_typedefs actually pulls the typedef out of the debugged-enabled binary, and on Unix those functions aren't used by default. This is spelled out in the pgindent/README in CVS. I could just EXEC_BACKEND in the debug build I use but I suppose there are other typedef I am missing as well. Any idea on a more comprehensive solution to finding typedefs? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Alvaro Herrera wrote:
>> It seems pgindent is not considering EXEC_BACKEND typedefs.
> Yep. The cause is that find_typedefs actually pulls the typedef out of
> the debugged-enabled binary, and on Unix those functions aren't used by
> default. This is spelled out in the pgindent/README in CVS.
> I could just EXEC_BACKEND in the debug build I use but I suppose there
> are other typedef I am missing as well. Any idea on a more
> comprehensive solution to finding typedefs?
I guess that explains why plpython.c and most of contrib have similar
problems.
If you want to do this on the basis of precompiled code, you need to
enable every optional feature in your build, and include the PLs and
contrib modules not only the core backend.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Alvaro Herrera wrote: > >> It seems pgindent is not considering EXEC_BACKEND typedefs. > > > Yep. The cause is that find_typedefs actually pulls the typedef out of > > the debugged-enabled binary, and on Unix those functions aren't used by > > default. This is spelled out in the pgindent/README in CVS. > > > I could just EXEC_BACKEND in the debug build I use but I suppose there > > are other typedef I am missing as well. Any idea on a more > > comprehensive solution to finding typedefs? > > I guess that explains why plpython.c and most of contrib have similar > problems. > > If you want to do this on the basis of precompiled code, you need to > enable every optional feature in your build, and include the PLs and > contrib modules not only the core backend. I do, I think. The problem with plpython is that my operating system's python it too old to compile it. I guess I could upgrade the python. I have added documentation to install /contrib libraries before finding typedefs so that should fix that problem. Does someone want to generate that typedef list in the future? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Does someone want to generate that typedef list in the future? I would love to do that, but objdump --stabs does not work for me like find_typedefs says it should. I only get something like: $ objdump --stabs ../backend/postgres ../backend/postgres: file format elf64-x86-64 $ objdump --version GNU objdump (GNU Binutils for Debian) 2.18.20071027 Copyright 2007 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or (at your option) any later version. This program has absolutely no warranty. I don't know how to make it output the symbol names like it seems to do for you. Having the typedef list in the script itself seems like a barrier for other people to contribute to this thing. I wonder if that can be changed so that the typedef is on a separate list. (Why are we still distributing pgjindent anyway?) I am also wondering if current GNU indent is now better suited to the task. Perhaps the bugs that it had on earlier versions have since been fixed? I remember checking the source code size a couple of years ago and it had grown by an order of magnitude or something like that. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> I don't know how to make it output the symbol names like it seems to do
> for you.
I dislike the object-file-based approach altogether, not least because
it appears to depend on unportable aspects of someBSD's objdump.
Surely there's some code out there that can find typedef names from the
source files? Why does pgindent even need to be told this?
> I am also wondering if current GNU indent is now better suited to the
> task. Perhaps the bugs that it had on earlier versions have since been
> fixed? I remember checking the source code size a couple of years ago
> and it had grown by an order of magnitude or something like that.
It might be interesting to try it again. It's been quite a few years
since anyone tried it on the PG sources, AFAIK.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > I don't know how to make it output the symbol names like it seems to do > > for you. > > I dislike the object-file-based approach altogether, not least because > it appears to depend on unportable aspects of someBSD's objdump. > > Surely there's some code out there that can find typedef names from the > source files? Why does pgindent even need to be told this? indent needs the typedef list. Maybe we can hack something based on typedefs in the source code, instead of object files. > > I am also wondering if current GNU indent is now better suited to the > > task. Perhaps the bugs that it had on earlier versions have since been > > fixed? I remember checking the source code size a couple of years ago > > and it had grown by an order of magnitude or something like that. > > It might be interesting to try it again. It's been quite a few years > since anyone tried it on the PG sources, AFAIK. Right. I can't do it right now, but perhaps I can have a look at it later. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > I don't know how to make it output the symbol names like it seems to do > for you. > > Having the typedef list in the script itself seems like a barrier for > other people to contribute to this thing. I wonder if that can be > changed so that the typedef is on a separate list. Done and committed. > (Why are we still distributing pgjindent anyway?) Removed. > I am also wondering if current GNU indent is now better suited to the > task. Perhaps the bugs that it had on earlier versions have since been > fixed? I remember checking the source code size a couple of years ago > and it had grown by an order of magnitude or something like that. Yes, we should give that another try. I will keep this as an 8.4 item. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Alvaro Herrera wrote: > Tom Lane wrote: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > I don't know how to make it output the symbol names like it seems to do > > > for you. > > > > I dislike the object-file-based approach altogether, not least because > > it appears to depend on unportable aspects of someBSD's objdump. > > > > Surely there's some code out there that can find typedef names from the > > source files? Why does pgindent even need to be told this? > > indent needs the typedef list. Maybe we can hack something based on > typedefs in the source code, instead of object files. I am worried it will be too hard to find typedefs in complex cases where the typedef name is embedded: typedef void (*ClosePtr) (struct _archiveHandle * AH); The only think of is to grab typedefs from the object file and then also try to get them from the souce too and combine them and remove duplicates. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Alvaro Herrera wrote:
>> indent needs the typedef list. Maybe we can hack something based on
>> typedefs in the source code, instead of object files.
> The only think of is to grab typedefs from the object file and then also
> try to get them from the souce too and combine them and remove duplicates.
Something I noticed the other day is that pgindent doesn't seem to treat
"struct foo" or "union foo" as a type name, which is pretty silly
because no context is needed at all to recognize that. We tend not to
do that too much --- the project style is to use a typedef name --- but
there are some places that do it, particularly the regex code. For
instance there are extra spaces here:
static void
cmtreefree(struct colormap * cm, union tree * tree, int level) /* level number (top == 0)
ofthis block */
{
Fixable?
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Alvaro Herrera wrote:
> >> indent needs the typedef list. Maybe we can hack something based on
> >> typedefs in the source code, instead of object files.
>
> > The only think of is to grab typedefs from the object file and then also
> > try to get them from the souce too and combine them and remove duplicates.
>
> Something I noticed the other day is that pgindent doesn't seem to treat
> "struct foo" or "union foo" as a type name, which is pretty silly
> because no context is needed at all to recognize that. We tend not to
> do that too much --- the project style is to use a typedef name --- but
> there are some places that do it, particularly the regex code. For
> instance there are extra spaces here:
>
> static void
> cmtreefree(struct colormap * cm,
> union tree * tree,
> int level) /* level number (top == 0) of this block */
> {
>
> Fixable?
Yes, I found those are 't' STABS rather than "T" which are used in cases
where you do typedef struct {} name. The next pgindent will have those
typedefs you want.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
I have commited a change to src/tools/find_typedef that should allow it to run under Linux. The only difference I see is that some unused typedefs do not appear in the Linux version. --------------------------------------------------------------------------- Alvaro Herrera wrote: > Bruce Momjian wrote: > > > Does someone want to generate that typedef list in the future? > > I would love to do that, but objdump --stabs does not work for me like > find_typedefs says it should. I only get something like: > > $ objdump --stabs ../backend/postgres > > ../backend/postgres: file format elf64-x86-64 > > $ objdump --version > GNU objdump (GNU Binutils for Debian) 2.18.20071027 > Copyright 2007 Free Software Foundation, Inc. > This program is free software; you may redistribute it under the terms of > the GNU General Public License version 3 or (at your option) any later version. > This program has absolutely no warranty. > > > I don't know how to make it output the symbol names like it seems to do > for you. > > Having the typedef list in the script itself seems like a barrier for > other people to contribute to this thing. I wonder if that can be > changed so that the typedef is on a separate list. > > (Why are we still distributing pgjindent anyway?) > > I am also wondering if current GNU indent is now better suited to the > task. Perhaps the bugs that it had on earlier versions have since been > fixed? I remember checking the source code size a couple of years ago > and it had grown by an order of magnitude or something like that. > > -- > Alvaro Herrera http://www.CommandPrompt.com/ > The PostgreSQL Company - Command Prompt, Inc. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > > I have commited a change to src/tools/find_typedef that should allow it > to run under Linux. The only difference I see is that some unused > typedefs do not appear in the Linux version. However, I think pgindent only cares about the typedef references, not the definitions, so I think it might be fine --------------------------------------------------------------------------- > > --------------------------------------------------------------------------- > > Alvaro Herrera wrote: > > Bruce Momjian wrote: > > > > > Does someone want to generate that typedef list in the future? > > > > I would love to do that, but objdump --stabs does not work for me like > > find_typedefs says it should. I only get something like: > > > > $ objdump --stabs ../backend/postgres > > > > ../backend/postgres: file format elf64-x86-64 > > > > $ objdump --version > > GNU objdump (GNU Binutils for Debian) 2.18.20071027 > > Copyright 2007 Free Software Foundation, Inc. > > This program is free software; you may redistribute it under the terms of > > the GNU General Public License version 3 or (at your option) any later version. > > This program has absolutely no warranty. > > > > > > I don't know how to make it output the symbol names like it seems to do > > for you. > > > > Having the typedef list in the script itself seems like a barrier for > > other people to contribute to this thing. I wonder if that can be > > changed so that the typedef is on a separate list. > > > > (Why are we still distributing pgjindent anyway?) > > > > I am also wondering if current GNU indent is now better suited to the > > task. Perhaps the bugs that it had on earlier versions have since been > > fixed? I remember checking the source code size a couple of years ago > > and it had grown by an order of magnitude or something like that. > > > > -- > > Alvaro Herrera http://www.CommandPrompt.com/ > > The PostgreSQL Company - Command Prompt, Inc. > > -- > Bruce Momjian <bruce@momjian.us> http://momjian.us > EnterpriseDB http://postgres.enterprisedb.com > > + If your life is a hard drive, Christ can be your backup. + > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +