Re: [HACKERS] Lessons from commit fest
I will try to look at it again in a few days. cheers andrew Bruce Momjian wrote: What is our plan for pgindent for 8.4? I would rather not have to bug someone to create a list of symbols manually. I would like it to be built on a regular basis and I can pull it from there and add it to CVS when I run pgindent. --- Andrew Dunstan wrote: Gregory Stark wrote: "Andrew Dunstan" <[EMAIL PROTECTED]> writes: Tom Lane wrote: doxygen's 200-some is clearly an order of magnitude too low, but I wonder whether Bruce's list hasn't got some false hits ... Skimming the output it does have things like "int" and "float" but presumably we would know if that caused any problem, they wouldn't inflate the numbers much. 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a build that is only missing the optional pam, bonjour and gssapi config options. The numbers going to vary heavily from OS to OS so it seems to me that these are a basically the same order of magnitude. It looks like Windows will blow all our existing numbers out of the water. Here's a list generated from Cygwin with 6088 symbols. I'm working on getting a similar list from MinGW. http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
What is our plan for pgindent for 8.4? I would rather not have to bug someone to create a list of symbols manually. I would like it to be built on a regular basis and I can pull it from there and add it to CVS when I run pgindent. --- Andrew Dunstan wrote: > > > Gregory Stark wrote: > > "Andrew Dunstan" <[EMAIL PROTECTED]> writes: > > > > > >> Tom Lane wrote: > >> > >>> doxygen's 200-some is clearly an order of magnitude too low, but I > >>> wonder whether Bruce's list hasn't got some false hits ... > >>> > > > > Skimming the output it does have things like "int" and "float" but > > presumably > > we would know if that caused any problem, they wouldn't inflate the numbers > > much. > > > > > >> 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 > >> on a > >> build that is only missing the optional pam, bonjour and gssapi config > >> options. > >> > > > > The numbers going to vary heavily from OS to OS so it seems to me that these > > are a basically the same order of magnitude. > > > > It looks like Windows will blow all our existing numbers out of the > water. Here's a list generated from Cygwin with 6088 symbols. I'm > working on getting a similar list from MinGW. > > http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs > > cheers > > andrew > > -- > 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 <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Does someone want to look at improving the pgindent script itself? > > I notice that you've carefully ignored the suggestion of re-testing > GNU indent. No. Why would I carefully ignore testing GNU indent? Because I am afraid pgindent will improve? Please, folks, start taking over the things I do: FAQs, TODO, pgindent, patch queue, whatever. I am tired of veiled complaints about how I handle things. I do my best. Let someone else handle them and then I can complain too. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: Bruce Momjian <[EMAIL PROTECTED]> writes: I have no problem using a URL to pull down the typedef list via wget. How is that CVS file going to be updated? I do not follow your thought process. You would rather depend on a URL that has no visible commit history? This does seem odd. Bruce if you want to use wget, point the wget at the cvsweb repo checkout. Joshua D. Drake -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian <[EMAIL PROTECTED]> writes: > Does someone want to look at improving the pgindent script itself? I notice that you've carefully ignored the suggestion of re-testing GNU indent. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian <[EMAIL PROTECTED]> writes: > I have no problem using a URL to pull down the typedef list via wget. > How is that CVS file going to be updated? I do not follow your thought process. You would rather depend on a URL that has no visible commit history? As I already noted elsewhere in the thread, the last thing we want is a typedef list that changes every five minutes. It *should* have adult supervision, and the effort of checking a vetted update into CVS seems entirely appropriate to me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> writes: > > It does take a while to run though ... it's not something we'll want to > > do routinely. > > Well, we're not going to want to change the reference typedef list very > often anyway, because it'd just result in whitespace-thrashing in the > repository. I'm thinking an update once or twice per major release > cycle would be enough. So a basically manual process that combines the > results from various buildfarm members, and then filters them like this, > should be workable. OK, fine, but when I do the full source tree pgindent, I will need a URL to get the list, so let's document its location in the README. Also, this improved typedef list is only making pgindent 0.2% better. Does someone want to look at improving the pgindent script itself? Might be a good time now that we have an updated typedef list for 8.4. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera <[EMAIL PROTECTED]> writes: > It does take a while to run though ... it's not something we'll want to > do routinely. Well, we're not going to want to change the reference typedef list very often anyway, because it'd just result in whitespace-thrashing in the repository. I'm thinking an update once or twice per major release cycle would be enough. So a basically manual process that combines the results from various buildfarm members, and then filters them like this, should be workable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Tom Lane wrote: > >> If we're going to go down this path, why would we not put the > >> "reference" typedef list into CVS? > > > Uh, I assume we don't want an automated system updating the file in CVS. > > Nowhere did I suggest that. What I suggested is that the "considered > good" reference list should be in CVS, not on some random wiki page. > In particular there's something pretty broken about the idea of a file > in CVS telling people to go to a wiki page for important data. I have no problem using a URL to pull down the typedef list via wget. How is that CVS file going to be updated? Is someone manually going to update it in CVS, and how often? -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Bruce Momjian <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> If we're going to go down this path, why would we not put the >> "reference" typedef list into CVS? > Uh, I assume we don't want an automated system updating the file in CVS. Nowhere did I suggest that. What I suggested is that the "considered good" reference list should be in CVS, not on some random wiki page. In particular there's something pretty broken about the idea of a file in CVS telling people to go to a wiki page for important data. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> writes: > > Andrew Dunstan wrote: > >> It looks like we'll need some sort of extra filter. > > > Hmm. Wow. For example I see > > > FINDREPLACE > > FINDREPLACEA > > FINDREPLACEW > > > We use neither ... My guess is that they are used in the system DLLs or > > something like that. > > Presumably we could grep our own sources for each proposed typedef list > entry --- no hits, you don't get in. Just came up with this: > found > not-found while read line ; do echo "looking for $line" rgrep -q --exclude cscope.out --exclude pgtypedefs.bsdos --exclude tags "\<$line\>" . if [ $? == 0 ]; then echo $line >> found else echo $line >> not-found fi done < pgtypedefs.bsdos It's simple enough that there are some false matches, for example for "AV" (which is a symbol we do use, but it also appears in strings etc). But I'd say it's more than enough. It does take a while to run though ... it's not something we'll want to do routinely. Okay, it finished: $ wc -l found not-found 2035 found 592 not-found 2627 total -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera wrote: > Bruce Momjian wrote: > > > I have created a proper typedef file that I would normally use for a > > pgindent run of the entire tree (it has /contrib, 2628 entries). It is > > at: > > > > http://momjian.us/expire/pgtypedefs.bsdos > > Well, there are typedefs in there not used anywhere in our code, for > example ASN1_GENERALIZEDTIME ... Yep. I assume the Win32 list is longer only because the Win32 API is larger. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > As soon as you have a stable typedef file we can all use please update > > the pgindent README to point to that URL. Keep the instructions of how > > to create it in our tree so we have it for future reference. > > If we're going to go down this path, why would we not put the > "reference" typedef list into CVS? Uh, I assume we don't want an automated system updating the file in CVS. I can' think of any CVS file that is updated in an automated manner like that. If a buildfarm member can't compile one day does it update with those entries missing, then re-add them the next day? -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > I have created a proper typedef file that I would normally use for a > pgindent run of the entire tree (it has /contrib, 2628 entries). It is > at: > > http://momjian.us/expire/pgtypedefs.bsdos Well, there are typedefs in there not used anywhere in our code, for example ASN1_GENERALIZEDTIME ... -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian <[EMAIL PROTECTED]> writes: > As soon as you have a stable typedef file we can all use please update > the pgindent README to point to that URL. Keep the instructions of how > to create it in our tree so we have it for future reference. If we're going to go down this path, why would we not put the "reference" typedef list into CVS? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Andrew Dunstan wrote: > > Skimming the output it does have things like "int" and "float" but > > presumably > > we would know if that caused any problem, they wouldn't inflate the numbers > > much. > > > > > >> 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 > >> on a > >> build that is only missing the optional pam, bonjour and gssapi config > >> options. > >> > > > > The numbers going to vary heavily from OS to OS so it seems to me that these > > are a basically the same order of magnitude. > > > > It looks like Windows will blow all our existing numbers out of the > water. Here's a list generated from Cygwin with 6088 symbols. I'm > working on getting a similar list from MinGW. > > http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs I have created a proper typedef file that I would normally use for a pgindent run of the entire tree (it has /contrib, 2628 entries). It is at: http://momjian.us/expire/pgtypedefs.bsdos Andrew, feel free to replace the typedef script in /tools with yours and we can all test it. I know mine worked on Ubuntu 7.10 and Alvaro got it working too. We can test your once you are ready. As soon as you have a stable typedef file we can all use please update the pgindent README to point to that URL. Keep the instructions of how to create it in our tree so we have it for future reference. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Andrew Dunstan wrote: >> It looks like we'll need some sort of extra filter. > Hmm. Wow. For example I see > FINDREPLACE > FINDREPLACEA > FINDREPLACEW > We use neither ... My guess is that they are used in the system DLLs or > something like that. Presumably we could grep our own sources for each proposed typedef list entry --- no hits, you don't get in. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Andrew Dunstan wrote: > And here are the 7625 from MinGW. > > http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dawn_bat&dt=2008-04-19%20004514&stg=typedefs > > It looks like we'll need some sort of extra filter. Hmm. Wow. For example I see FINDREPLACE FINDREPLACEA FINDREPLACEW We use neither ... My guess is that they are used in the system DLLs or something like that. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Andrew Dunstan wrote: Gregory Stark wrote: "Andrew Dunstan" <[EMAIL PROTECTED]> writes: Tom Lane wrote: doxygen's 200-some is clearly an order of magnitude too low, but I wonder whether Bruce's list hasn't got some false hits ... Skimming the output it does have things like "int" and "float" but presumably we would know if that caused any problem, they wouldn't inflate the numbers much. 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a build that is only missing the optional pam, bonjour and gssapi config options. The numbers going to vary heavily from OS to OS so it seems to me that these are a basically the same order of magnitude. It looks like Windows will blow all our existing numbers out of the water. Here's a list generated from Cygwin with 6088 symbols. I'm working on getting a similar list from MinGW. http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs And here are the 7625 from MinGW. http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dawn_bat&dt=2008-04-19%20004514&stg=typedefs It looks like we'll need some sort of extra filter. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> writes: It looks like Windows will blow all our existing numbers out of the water. Here's a list generated from Cygwin with 6088 symbols. I'm working on getting a similar list from MinGW. Hmm, your toolset must be listing all typedefs present in the header files, not just those actually used? No, this is from objdump --stabs, just as find_typedefs does. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Andrew Dunstan <[EMAIL PROTECTED]> writes: > It looks like Windows will blow all our existing numbers out of the > water. Here's a list generated from Cygwin with 6088 symbols. I'm > working on getting a similar list from MinGW. Hmm, your toolset must be listing all typedefs present in the header files, not just those actually used? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Gregory Stark wrote: "Andrew Dunstan" <[EMAIL PROTECTED]> writes: Tom Lane wrote: doxygen's 200-some is clearly an order of magnitude too low, but I wonder whether Bruce's list hasn't got some false hits ... Skimming the output it does have things like "int" and "float" but presumably we would know if that caused any problem, they wouldn't inflate the numbers much. 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a build that is only missing the optional pam, bonjour and gssapi config options. The numbers going to vary heavily from OS to OS so it seems to me that these are a basically the same order of magnitude. It looks like Windows will blow all our existing numbers out of the water. Here's a list generated from Cygwin with 6088 symbols. I'm working on getting a similar list from MinGW. http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=brown_bat&dt=2008-04-18%20230054&stg=typedefs cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
"Andrew Dunstan" <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> doxygen's 200-some is clearly an order of magnitude too low, but I >> wonder whether Bruce's list hasn't got some false hits ... Skimming the output it does have things like "int" and "float" but presumably we would know if that caused any problem, they wouldn't inflate the numbers much. > 2800 does seem a bit high. My buildfarm member dungbeetle just found 2482 on a > build that is only missing the optional pam, bonjour and gssapi config > options. The numbers going to vary heavily from OS to OS so it seems to me that these are a basically the same order of magnitude. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote:
Alvaro Herrera <[EMAIL PROTECTED]> writes:
Greg Smith wrote:
Scraping that HTML seems like it would be pretty straightforward.
It's awfully incomplete. Bruce said to me the other day on IM that the
list he was getting with the Linux version of find_typedef was something
like 2800 symbols. I checked the doxygen list and I only see about a
dozen for each letter, so there's a whole lot missing here.
[ click click... ] A quick grep counts 2154 occurrences of the word
'typedef' in our tree. Some of them are no doubt false hits
(documentation etc), but on the other hand you need to add typedefs
coming from system headers.
doxygen's 200-some is clearly an order of magnitude too low, but I
wonder whether Bruce's list hasn't got some false hits ...
2800 does seem a bit high. My buildfarm member dungbeetle just found
2482 on a build that is only missing the optional pam, bonjour and
gssapi config options.
Here's the list already loaded to the server:
http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=dungbeetle&dt=2008-04-18%20160103&stg=typedefs
I had to change the logic some - the stuff in the find_typedefs script
seemed to be quite broken on my Linux box (fairly vanilla fc6).
The relevant perl code is below.
I'll see about running this with Windows and Cygwin and I can even try OSX.
cheers
andrew
sub find_typedefs
{
my @err = `objdump -W 2>&1`;
my %syms;
my @dumpout;
my @flds;
foreach my $bin (glob("$installdir/bin/*"),
glob("$installdir/lib/*"),
glob("$installdir/lib/postgresql/*"))
{
next if $bin =~ m!bin/(ipcclean|pltcl_)!;
next unless -f $bin;
if (@err == 1) # Linux
{
@dumpout = `objdump -W $bin 2>/dev/null | egrep -A3
'(DW_TAG_typedef|DW_TAG_structure_type|DW_TAG_union_type)' 2>/dev/null`;
foreach (@dumpout)
{
@flds = split;
next if ($flds[0] ne 'DW_AT_name' || $flds[-1] =~
/^DW_FORM_str/);
$syms{$flds[-1]} =1;
}
}
else
{
@dumpout = `objdump --stabs $bin 2>/dev/null`;
foreach (@dumpout)
{
@flds = split;
next if ($flds[1] ne 'LSYM' || $flds[6] !~ /([^:]+):[tT]/);
$syms{$1} =1;
}
}
}
my @badsyms = grep { /\s/ } keys %syms;
push(@badsyms,'date','interval','timestamp','ANY');
delete @[EMAIL PROTECTED];
my @goodsyms = sort keys %syms;
map { s/$/\n/; } @goodsyms;
writelog('typedefs',[EMAIL PROTECTED]);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera wrote: > Bruce Momjian wrote: > > > pgindent is probably 97% optimal. Getting a better typedef list will > > change that to perhaps 97.2% optimal. There is a lot of discussion > > happening to try to get that 0.2%. :-O > > If I'm allowed to make my own guesses I'd say pgindent is at about 90% > currently and we could get it to 99% or more by leveraging the > buildfarm. My point is that typedefs are only a small part of what makes pgindent less than perfect. I should have said a "perfect typedef list" would make it 97.2%. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > pgindent is probably 97% optimal. Getting a better typedef list will > change that to perhaps 97.2% optimal. There is a lot of discussion > happening to try to get that 0.2%. :-O If I'm allowed to make my own guesses I'd say pgindent is at about 90% currently and we could get it to 99% or more by leveraging the buildfarm. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > [ click click... ] A quick grep counts 2154 occurrences of the word > 'typedef' in our tree. Some of them are no doubt false hits > (documentation etc), but on the other hand you need to add typedefs > coming from system headers. > > doxygen's 200-some is clearly an order of magnitude too low, but I > wonder whether Bruce's list hasn't got some false hits ... You also have to account for typedefs in OpenSSL headers, Kerberos, readline, etc. Perhaps you can count them as "system headers", but then it starts to be clear that you can't just process our own source files, or any system's headers alone. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> writes: > > Greg Smith wrote: > >> Scraping that HTML seems like it would be pretty straightforward. > > > It's awfully incomplete. Bruce said to me the other day on IM that the > > list he was getting with the Linux version of find_typedef was something > > like 2800 symbols. I checked the doxygen list and I only see about a > > dozen for each letter, so there's a whole lot missing here. > > [ click click... ] A quick grep counts 2154 occurrences of the word > 'typedef' in our tree. Some of them are no doubt false hits > (documentation etc), but on the other hand you need to add typedefs > coming from system headers. > > doxygen's 200-some is clearly an order of magnitude too low, but I > wonder whether Bruce's list hasn't got some false hits ... My list is at: http://momjian.us/tmp/pgtypedefs pgindent is probably 97% optimal. Getting a better typedef list will change that to perhaps 97.2% optimal. There is a lot of discussion happening to try to get that 0.2%. :-O -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Greg Smith wrote: >> Scraping that HTML seems like it would be pretty straightforward. > It's awfully incomplete. Bruce said to me the other day on IM that the > list he was getting with the Linux version of find_typedef was something > like 2800 symbols. I checked the doxygen list and I only see about a > dozen for each letter, so there's a whole lot missing here. [ click click... ] A quick grep counts 2154 occurrences of the word 'typedef' in our tree. Some of them are no doubt false hits (documentation etc), but on the other hand you need to add typedefs coming from system headers. doxygen's 200-some is clearly an order of magnitude too low, but I wonder whether Bruce's list hasn't got some false hits ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Greg Smith wrote: > On Fri, 18 Apr 2008, Gregory Stark wrote: > >> The reason I was asking these questions was because I was thinking >> about how hard it would be to generate the list from a textual analysis >> instead of using object files. > > Is there some reason I don't understand why the listing doyxgen creates > isn't good enough here? http://doxygen.postgresql.org/globals_type.html > > Scraping that HTML seems like it would be pretty straightforward. It's awfully incomplete. Bruce said to me the other day on IM that the list he was getting with the Linux version of find_typedef was something like 2800 symbols. I checked the doxygen list and I only see about a dozen for each letter, so there's a whole lot missing here. -- Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4 "The Gord often wonders why people threaten never to come back after they've been told never to return" (www.actsofgord.com) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
On Fri, 18 Apr 2008, Gregory Stark wrote: The reason I was asking these questions was because I was thinking about how hard it would be to generate the list from a textual analysis instead of using object files. Is there some reason I don't understand why the listing doyxgen creates isn't good enough here? http://doxygen.postgresql.org/globals_type.html Scraping that HTML seems like it would be pretty straightforward. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Gregory Stark wrote: But if we're still doing object file analysis on the build farm and it's easy to add typedefs manually then perhaps there's not much effort saved by having such a tool. I think it would be possible to write but it wouldn't really be easy. You have to parse just enough C to find the typedef but not so much you get confused by invalid C syntax caused by looking at both sides of #ifdef branches. I am pretty dead sure that a textual analysis tool is going to be far too much work to write and maintain, for the benefit we might get from it. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
"Tom Lane" <[EMAIL PROTECTED]> writes: "Tom Lane" <[EMAIL PROTECTED]> writes: > Gregory Stark <[EMAIL PROTECTED]> writes: >> "Tom Lane" <[EMAIL PROTECTED]> writes: > >> 1) I take it we feel safe guaranteeing that we won't use any fancy macros >>inside typedefs. So no '#define pgtype(x) _pg_##x' or anythin like that. > > Hmm ... we are fairly crawling with struct tags built that way: > > /* Introduces a catalog's structure definition */ > #define CATALOG(name,oid) typedef struct CppConcat(FormData_,name) > > but offhand I can't think of any actual typedef names built with ##. > Does indent need a preset list of struct tags? Seems that would be > stupid ... It's not just ## that's a problem. Any macro used to build the typedef would be a problem. There's not a whole lot of other reasons you would want to use macros in a typedef but... >> 3) How would this work with typedefs which come from system or library >>includes? > > Ouch, that's a real good point. Maybe a certain amount of platform > dependence is inevitable. The reason I was asking these questions was because I was thinking about how hard it would be to generate the list from a textual analysis instead of using object files. Such a tool *cannot* use cpp to preprocess the file because it would defeat much of the purpose. The point is that we want to find all the typedefs in all the #ifdef branches. But if we don't preprocess the files with CPP then macros like the one I included before wouldn't be interpreted. Nor would we be pulling in system or library headers, so no typedefs from them. But if we're just interested in the names I suppose a hybrid approach would work. 1) The build farm builds a list of typedefs found in all the various builds and we check that into CVS. 2) a textual tool run as part of your normal build builds a list of typedefs found in your tree. But if we're still doing object file analysis on the build farm and it's easy to add typedefs manually then perhaps there's not much effort saved by having such a tool. I think it would be possible to write but it wouldn't really be easy. You have to parse just enough C to find the typedef but not so much you get confused by invalid C syntax caused by looking at both sides of #ifdef branches. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
* Tom Lane <[EMAIL PROTECTED]> [080417 20:47]: > Right, but if the only use is inside #ifdef __BRAND_X_PLATFORM__, > the only way for the proposed toolchain to extract that usage is > if someone runs it on BRAND_X_PLATFORM. > > Of course, for seldom-used platforms maybe we don't care that much. But if we're talking about putting a "definitive list" of them in the source, we can build that list in a few iterations, with any method we want (including using Bruce's list as a starting point, some inspection, some grep/awk/perl, etc). It's not something that will need to be re-run on a continual basis and produce a definitive list each and every run. a. -- Aidan Van Dyk Create like a god, [EMAIL PROTECTED] command like a king, http://www.highrise.ca/ work like a slave. signature.asc Description: Digital signature
Re: [HACKERS] Lessons from commit fest
Aidan Van Dyk <[EMAIL PROTECTED]> writes: > * Tom Lane <[EMAIL PROTECTED]> [080417 20:11]: >> Ouch, that's a real good point. Maybe a certain amount of platform >> dependence is inevitable. > I don't get it. If they are used as typedefs *anywhere* in the PG > source, they're needed in the typedef list. Right, but if the only use is inside #ifdef __BRAND_X_PLATFORM__, the only way for the proposed toolchain to extract that usage is if someone runs it on BRAND_X_PLATFORM. Of course, for seldom-used platforms maybe we don't care that much. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
* Tom Lane <[EMAIL PROTECTED]> [080417 20:11]: > > 3) How would this work with typedefs which come from system or library > >includes? > > Ouch, that's a real good point. Maybe a certain amount of platform > dependence is inevitable. I don't get it. If they are used as typedefs *anywhere* in the PG source, they're needed in the typedef list. If they are not used in the PG source, they aren't needed in the typedef list. -- Aidan Van Dyk Create like a god, [EMAIL PROTECTED] command like a king, http://www.highrise.ca/ work like a slave. signature.asc Description: Digital signature
Re: [HACKERS] Lessons from commit fest
Gregory Stark <[EMAIL PROTECTED]> writes: > "Tom Lane" <[EMAIL PROTECTED]> writes: >> That would certainly be better than the current approach, since >> presumably it would cover not only Windows but the other >> conditionally-compiled stuff that Bruce chooses not to compile on >> his own machine. > It would, as someone said, rock. But it wouldn't really address the ability of > a developer to run pgindent on code he's about to send in, since it wouldn't > have any typedefs that developer just created. Well, that list is just a simple text file listing typedef names, so it'd hardly be difficult to add your own to the list. >> I still wish we could build the list directly from the source code, >> but I have no suggestions for tools that would do it. > If we wanted to do that I have a few questions: > 1) I take it we feel safe guaranteeing that we won't use any fancy macros >inside typedefs. So no '#define pgtype(x) _pg_##x' or anythin like that. Hmm ... we are fairly crawling with struct tags built that way: /* Introduces a catalog's structure definition */ #define CATALOG(name,oid) typedef struct CppConcat(FormData_,name) but offhand I can't think of any actual typedef names built with ##. Does indent need a preset list of struct tags? Seems that would be stupid ... > 2) How much information do we need about the typedefs? Just their name? Right. > 3) How would this work with typedefs which come from system or library >includes? Ouch, that's a real good point. Maybe a certain amount of platform dependence is inevitable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
"Tom Lane" <[EMAIL PROTECTED]> writes: > Andrew Dunstan <[EMAIL PROTECTED]> writes: >> I have been thinking of pursuing your suggestion of having it as a >> buildfarm option. We could provide a SOAP interface to collect the >> typedefs and then consolidate them and put them in CVS. We could even do >> it per release. That would include Windows, although only MinGW, not >> MSVC, which doesn't have objdump. > > That would certainly be better than the current approach, since > presumably it would cover not only Windows but the other > conditionally-compiled stuff that Bruce chooses not to compile on > his own machine. It would, as someone said, rock. But it wouldn't really address the ability of a developer to run pgindent on code he's about to send in, since it wouldn't have any typedefs that developer just created. > I still wish we could build the list directly from the source code, > but I have no suggestions for tools that would do it. If we wanted to do that I have a few questions: 1) I take it we feel safe guaranteeing that we won't use any fancy macros inside typedefs. So no '#define pgtype(x) _pg_##x' or anythin like that. 2) How much information do we need about the typedefs? Just their name? 3) How would this work with typedefs which come from system or library includes? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
[EMAIL PROTECTED] (Tom Lane) writes: > Chris Browne <[EMAIL PROTECTED]> writes: >> Would it be a terrible idea to... >> >> - Draw the indent code from NetBSD into src/tools/pgindent > > I am not real eager to become maintainers of our own indent fork, which > is what you propose. (Just for starters, what will we have to do to > make it run on non-BSD systems?) > >> We are presently at the extreme position where pgindent is run once in >> a very long time (~ once a year), at pretty considerable cost, and >> with the associated cost that a whole lot of indentation problems are >> managed by hand. > > Yeah. One reason for that is that the typedef problem makes it a pretty > manual process. As I hear more about the "typedef problem," a part of me gets more and more appalled... It seems like we're creating some problem for ourselves in that the typedefs don't seem to be able to be consistent. I don't have an answer, but it's looking like a sore tooth that clearly needs attention. > The main problem I see with "pgindent early and often" is that it only > works well if everyone is using exactly the same pgindent code (and > exactly the same typedef list). Otherwise you just get buried in > useless whitespace diffs. > > It's bad enough that Bruce whacks around his copy from time to time :-(. > I would say that the single greatest annoyance for maintaining our back > branches is that patches tend to not back-patch cleanly, and well over > half the time it's because of random reformattings done by pgindent > to code that hadn't changed at all, but it had formatted differently > the prior year. > > For the same reason, my take on your "random whitespace changes are > acceptable" theory is not no but hell no. It's gonna cost us, > permanently, in manual patch adjustments if we allow the repository to > get cluttered with content-free diffs. I don't want to be cavalier about it; I'm hoping that in the discussion, some more stable answer may fall out. Though with the typedef issues that have emerged, I'm not entirely sanguine... -- (reverse (concatenate 'string "gro.mca" "@" "enworbbc")) http://linuxfinances.info/info/internet.html HEADLINE: Suicidal twin kills sister by mistake! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Andrew Dunstan <[EMAIL PROTECTED]> writes: > I have been thinking of pursuing your suggestion of having it as a > buildfarm option. We could provide a SOAP interface to collect the > typedefs and then consolidate them and put them in CVS. We could even do > it per release. That would include Windows, although only MinGW, not > MSVC, which doesn't have objdump. That would certainly be better than the current approach, since presumably it would cover not only Windows but the other conditionally-compiled stuff that Bruce chooses not to compile on his own machine. Note though that the existing find_typedef script only works on (some variants of) Linux and BSD. Porting it to a wider set of platforms might be pretty painful. I still wish we could build the list directly from the source code, but I have no suggestions for tools that would do it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Gregory Stark wrote: > The only thing is that if the whole point is to have patch submitters run > pgindent on their own added code it won't work since their own code will be > precisely the code with the missing typedefs. How easy is it to manually add a > handful of typedefs to the list? The list is just a plain text file with one typedef per line, so it should be trivial. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Andrew Dunstan wrote: > I have been thinking of pursuing your suggestion of having it as a > buildfarm option. We could provide a SOAP interface to collect the > typedefs and then consolidate them and put them in CVS. We could even do > it per release. That would include Windows, although only MinGW, not > MSVC, which doesn't have objdump. That would rock. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
"Alvaro Herrera" <[EMAIL PROTECTED]> writes: > Bruce Momjian wrote: > >> Based on that reaction I am not going to bother uploading my copy of the >> typedefs. > > Please reconsider. Not having pgindent work at all is not better than > it working "only" 98%. I think I'm rescinding my objection to checking a canonical list of typedefs into CVS. I didn't realize how hard it was to generate these typedefs or how important it was to have everyone using the same version. Since we really *don't* want individual developers rebuilding the list of typedefs we don't have to worry about conflicts when the upstream list changes. Bruce's list of typedefs seems like a good start point for a "canonical" list of typedefs. The idea of gathering the lists from the build farm and consolidating them sounds like a good plan going forward. The only thing is that if the whole point is to have patch submitters run pgindent on their own added code it won't work since their own code will be precisely the code with the missing typedefs. How easy is it to manually add a handful of typedefs to the list? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's RemoteDBA services! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera wrote: Bruce Momjian wrote: Based on that reaction I am not going to bother uploading my copy of the typedefs. Please reconsider. Not having pgindent work at all is not better than it working "only" 98%. I have been thinking of pursuing your suggestion of having it as a buildfarm option. We could provide a SOAP interface to collect the typedefs and then consolidate them and put them in CVS. We could even do it per release. That would include Windows, although only MinGW, not MSVC, which doesn't have objdump. I could probably get most of that done in a day or so. Thoughts? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera wrote: > Bruce Momjian wrote: > > > Based on that reaction I am not going to bother uploading my copy of the > > typedefs. > > Please reconsider. Not having pgindent work at all is not better than > it working "only" 98%. That's what I thought, but Tom thinks my list is unacceptable. What do others think? -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > Based on that reaction I am not going to bother uploading my copy of the > typedefs. Please reconsider. Not having pgindent work at all is not better than it working "only" 98%. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> writes: > > What are we going to do about the duality of Windows vs. non-Windows? > > Perhaps we could collect typedefs generated on the buildfarm. > > I think it's really not acceptable that pgindent misformats Windows-only > code (or any other part of the code that Bruce doesn't care enough about > to build on his system). > > This whole business of extracting the typedef names from object files > sucks to begin with, and it will never not suck. If we have to have > such a list, we need to find a more platform-independent procedure > for getting it. Based on that reaction I am not going to bother uploading my copy of the typedefs. "If it can't be perfect, don't bother doing it" seems to be the approach, and because pgindent will never be perfect, I don't understand why we bother even running it. Not finding all typedefs is only part of the imperfect-ness of pgindent. I think we use pgindent and an imperfect typedef list because it has proven to be "good enough" to be useful. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera <[EMAIL PROTECTED]> writes: > What are we going to do about the duality of Windows vs. non-Windows? > Perhaps we could collect typedefs generated on the buildfarm. I think it's really not acceptable that pgindent misformats Windows-only code (or any other part of the code that Bruce doesn't care enough about to build on his system). This whole business of extracting the typedef names from object files sucks to begin with, and it will never not suck. If we have to have such a list, we need to find a more platform-independent procedure for getting it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > The source code is the same for both Unix and Windows but you are right > some typedefs are only visible on windows. I think most are from > EXEC_BACKEND so compiling with/without that should help but then you > have to merge the typedef lists, of course. The source code is the same, of course, but typedef generation uses object files, not source code. > I count 2481 typedefs found on my build. I don't think we have to find > every single typedef in the system for pgindent to be useful, but if we > want people to be able to use this we should choose a single typedef > file and all use that. I am willing to create a standard one for > everyone and upload it daily to the community ftp server. It will not > be perfect but I can improve it as people make suggestions. Please do. What are we going to do about the duality of Windows vs. non-Windows? Perhaps we could collect typedefs generated on the buildfarm. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera wrote: > Bruce Momjian wrote: > > Magnus Hagander wrote: > > > > IIRC, last time I tried it, the failure was because I couldn't get it > > > to build the proper typedefs. Any chance you could just put a regularly > > > updated typedefs file somewhere that I could wget down? > > > > Have you tried the CVS version? It should support typedefs on Linux? I > > can put a continually-updated version on my ftp site if people want it. > > A typedef file generated from a single build is no good. We need at > least one for a regular Unix and another one from Windows. The set of > typedefs is different. > > If everyone is expected to generate the typedef on their local builds, > then the pgindent output will be different for every developer, thus > generating lots of spurious changes. This is not acceptable nor useful. The source code is the same for both Unix and Windows but you are right some typedefs are only visible on windows. I think most are from EXEC_BACKEND so compiling with/without that should help but then you have to merge the typedef lists, of course. I count 2481 typedefs found on my build. I don't think we have to find every single typedef in the system for pgindent to be useful, but if we want people to be able to use this we should choose a single typedef file and all use that. I am willing to create a standard one for everyone and upload it daily to the community ftp server. It will not be perfect but I can improve it as people make suggestions. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Magnus Hagander wrote: > > > > Also I can put up a web page where you can upload or email your C > > > > file and get a formatted version back. > > > > > > IIRC, last time I tried it, the failure was because I couldn't get > > > it to build the proper typedefs. Any chance you could just put a > > > regularly updated typedefs file somewhere that I could wget down? > > > > Have you tried the CVS version? It should support typedefs on > > Linux? I can put a continually-updated version on my ftp site if > > people want it. > > Right, but it requires me to run configure with all modules enabled, > right? And I don't have all modules installed on all machines, etc etc I don't enable all modules actually because I don't have everything installed either, but I do my best. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > Magnus Hagander wrote: > > IIRC, last time I tried it, the failure was because I couldn't get it > > to build the proper typedefs. Any chance you could just put a regularly > > updated typedefs file somewhere that I could wget down? > > Have you tried the CVS version? It should support typedefs on Linux? I > can put a continually-updated version on my ftp site if people want it. A typedef file generated from a single build is no good. We need at least one for a regular Unix and another one from Windows. The set of typedefs is different. If everyone is expected to generate the typedef on their local builds, then the pgindent output will be different for every developer, thus generating lots of spurious changes. This is not acceptable nor useful. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > Magnus Hagander wrote: > > Bruce Momjian wrote: > > > Bruce Momjian wrote: > > > > Tom Lane wrote: > > > > > Bruce Momjian <[EMAIL PROTECTED]> writes: > > > > > > I am reviewing the psql wrap patch and just used pgindent > > > > > > today to clean it up. (pgindent did not add any extra > > > > > > spacing changes.) Patch reviewers should probably be able > > > > > > to run pgindent. > > > > > > > > > > Well, that means nobody in the world can review except you, > > > > > because nobody else has ever reported success in duplicating > > > > > your pgindent results. I know I haven't been able to. > > > > > > > > > > If you really believe the above then you need to try a bit > > > > > harder to find a portable version of indent that we all can > > > > > use. > > > > > > > > The source code of pgindent I use is on our ftp site. > > > > find_typedefs should now work on Linux as well as BSD. Not > > > > sure what else would be a problem. > > > > > > Also I can put up a web page where you can upload or email your C > > > file and get a formatted version back. > > > > IIRC, last time I tried it, the failure was because I couldn't get > > it to build the proper typedefs. Any chance you could just put a > > regularly updated typedefs file somewhere that I could wget down? > > Have you tried the CVS version? It should support typedefs on > Linux? I can put a continually-updated version on my ftp site if > people want it. Right, but it requires me to run configure with all modules enabled, right? And I don't have all modules installed on all machines, etc etc (And I didn't get it working properly either way, but that could be because I simply didn't try hard enough) //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Magnus Hagander wrote: > Bruce Momjian wrote: > > Bruce Momjian wrote: > > > Tom Lane wrote: > > > > Bruce Momjian <[EMAIL PROTECTED]> writes: > > > > > I am reviewing the psql wrap patch and just used pgindent today > > > > > to clean it up. (pgindent did not add any extra spacing > > > > > changes.) Patch reviewers should probably be able to run > > > > > pgindent. > > > > > > > > Well, that means nobody in the world can review except you, > > > > because nobody else has ever reported success in duplicating your > > > > pgindent results. I know I haven't been able to. > > > > > > > > If you really believe the above then you need to try a bit harder > > > > to find a portable version of indent that we all can use. > > > > > > The source code of pgindent I use is on our ftp site. find_typedefs > > > should now work on Linux as well as BSD. Not sure what else would > > > be a problem. > > > > Also I can put up a web page where you can upload or email your C file > > and get a formatted version back. > > IIRC, last time I tried it, the failure was because I couldn't get it > to build the proper typedefs. Any chance you could just put a regularly > updated typedefs file somewhere that I could wget down? Have you tried the CVS version? It should support typedefs on Linux? I can put a continually-updated version on my ftp site if people want it. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Peter Eisentraut wrote: > Martijn van Oosterhout wrote: > > I doubt it, indent doesn't know nearly enough C to be able to anything > > other than adjust whitespace. It surely won't remove braces... > > I faintly recall that it does or at least did at some point. It used to remove braces around single-statement blocks, but that feature was removed because it broke indentation of PG_TRY blocks. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Martijn van Oosterhout wrote: > I doubt it, indent doesn't know nearly enough C to be able to anything > other than adjust whitespace. It surely won't remove braces... I faintly recall that it does or at least did at some point. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > Bruce Momjian wrote: > > Tom Lane wrote: > > > Bruce Momjian <[EMAIL PROTECTED]> writes: > > > > I am reviewing the psql wrap patch and just used pgindent today > > > > to clean it up. (pgindent did not add any extra spacing > > > > changes.) Patch reviewers should probably be able to run > > > > pgindent. > > > > > > Well, that means nobody in the world can review except you, > > > because nobody else has ever reported success in duplicating your > > > pgindent results. I know I haven't been able to. > > > > > > If you really believe the above then you need to try a bit harder > > > to find a portable version of indent that we all can use. > > > > The source code of pgindent I use is on our ftp site. find_typedefs > > should now work on Linux as well as BSD. Not sure what else would > > be a problem. > > Also I can put up a web page where you can upload or email your C file > and get a formatted version back. IIRC, last time I tried it, the failure was because I couldn't get it to build the proper typedefs. Any chance you could just put a regularly updated typedefs file somewhere that I could wget down? //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
On Thu, Apr 17, 2008 at 09:11:12AM +0300, Heikki Linnakangas wrote:
> Something like this:
>
> if (foo)
> {
> do something;
> do something else;
> }
> ...
>
> ->
>
> if (foo)
> do something;
> do something else;
> ...
I doubt it, indent doesn't know nearly enough C to be able to anything
other than adjust whitespace. It surely won't remove braces...
Have a nice day,
--
Martijn van Oosterhout <[EMAIL PROTECTED]> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.
signature.asc
Description: Digital signature
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera wrote:
Bruce Momjian wrote:
Alvaro Herrera wrote:
Maybe this means that we should give pgindent a run over the back
branches.
Yea, that thought has crossed our minds, but the problem is that there
is little testing of back branches so it would be risky.
That's a fair point, though I wonder how can a code indenter be so
broken that it broke code in ways not detected by our buildfarm.
Something like this:
if (foo)
{
do something;
do something else;
}
...
->
if (foo)
do something;
do something else;
...
I wouldn't be too surprised if something like that happened with one of
the complex macros, like PG_TRY/CATCH.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <[EMAIL PROTECTED]> writes: > > > I am reviewing the psql wrap patch and just used pgindent today to clean > > > it up. (pgindent did not add any extra spacing changes.) Patch > > > reviewers should probably be able to run pgindent. > > > > Well, that means nobody in the world can review except you, because > > nobody else has ever reported success in duplicating your pgindent > > results. I know I haven't been able to. > > > > If you really believe the above then you need to try a bit harder > > to find a portable version of indent that we all can use. > > The source code of pgindent I use is on our ftp site. find_typedefs > should now work on Linux as well as BSD. Not sure what else would be a > problem. Also I can put up a web page where you can upload or email your C file and get a formatted version back. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > I am reviewing the psql wrap patch and just used pgindent today to clean > > it up. (pgindent did not add any extra spacing changes.) Patch > > reviewers should probably be able to run pgindent. > > Well, that means nobody in the world can review except you, because > nobody else has ever reported success in duplicating your pgindent > results. I know I haven't been able to. > > If you really believe the above then you need to try a bit harder > to find a portable version of indent that we all can use. The source code of pgindent I use is on our ftp site. find_typedefs should now work on Linux as well as BSD. Not sure what else would be a problem. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Bruce Momjian <[EMAIL PROTECTED]> writes: > I am reviewing the psql wrap patch and just used pgindent today to clean > it up. (pgindent did not add any extra spacing changes.) Patch > reviewers should probably be able to run pgindent. Well, that means nobody in the world can review except you, because nobody else has ever reported success in duplicating your pgindent results. I know I haven't been able to. If you really believe the above then you need to try a bit harder to find a portable version of indent that we all can use. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
"Alvaro Herrera" <[EMAIL PROTECTED]> writes: > I suggested to you awhile back that we could keep a typedef file on the > repository, saving one step. That kind of sucks since it means you get conflicts when you update if you've run it yourself. Is there a reason we can't write makefiles which generate this file? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's RemoteDBA services! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Chris Browne wrote: > > That is much a more radical use of pgindent than it has had in the past > > but it is certainly possible. > > Well, supposing you're cleaning up a patch after someone has generated > it in bad style, it would seem like rather less work to use pgindent > to impose style policy right away rather than simulating its effects > manually. I am reviewing the psql wrap patch and just used pgindent today to clean it up. (pgindent did not add any extra spacing changes.) Patch reviewers should probably be able to run pgindent. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Bruce Momjian wrote: >> Alvaro Herrera wrote: >>> Maybe this means that we should give pgindent a run over the back >>> branches. >> >> Yea, that thought has crossed our minds, but the problem is that there >> is little testing of back branches so it would be risky. > That's a fair point, though I wonder how can a code indenter be so > broken that it broke code in ways not detected by our buildfarm. Yeah, the buildfarm has changed that equation a little. I think that if we were going to do something like switch to GNU indent, we'd really have to do that (re-indent the back branches) to maintain our sanity. And again anytime we moved to a new release of indent. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
[EMAIL PROTECTED] (Bruce Momjian) writes: > Chris Browne wrote: >> >> Would it be a terrible idea to... >> >> >> >> - Draw the indent code from NetBSD into src/tools/pgindent >> >> - Build it _in place_ inside the code tree (e.g. - don't assume >> >> it will get installed in /usr/local/bin) >> >> - Thus have the ability to run it in place? >> > >> > Yes, but it bloats our code and people still need to generate the >> > typedefs and follow the instructions. The other problem is if they run >> > it on a file they have modified, it is going to adjust places they >> > didn't touch, thereby making the patch harder to review. >> >> The bloat is 154K, on a project with something around 260MB of code. >> I don't think this is a particlarly material degree of bloat. > > You mean 37kb vs 13MB, right? That is the tarball sizes I see. Hmm. I was apparently badly counting the size of the sources. I ran a find | wc command that seemed to report 260MB of code. A "du" on a "make distclean" tree gives me 104MB. At any rate, that's only out by a bit more than a binary order of magnitude ;-). I was thinking about the 154K of source code sitting in CVS, not the (yes, lower) cost of it in a tarball. Seems immaterial either way... >> If we ran pgindent really frequently, there would admittedly be the >> difference that there would be a lot of little cases of >> changes-from-pgindent being committed along the way, but [1] might it >> not be cheaper to accept that cost, with the concomittant benefit that >> you could tell patchers "Hey, run pgindent before submitting that >> patch, and that'll clean up a number of of the issues." Yes, it >> doesn't address code changes like typedef generation, but that never >> was an argument against running pgindent... > > That is much a more radical use of pgindent than it has had in the past > but it is certainly possible. Well, supposing you're cleaning up a patch after someone has generated it in bad style, it would seem like rather less work to use pgindent to impose style policy right away rather than simulating its effects manually. I'm not proposing anything *really* radical, like migrating away from CVS, after all! ;-) -- let name="cbbrowne" and tld="linuxfinances.info" in String.concat "@" [name;tld];; http://linuxfinances.info/info/lisp.html There was a young lady of Crewe Whose limericks stopped at line two. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > Alvaro Herrera wrote: > > Maybe this means that we should give pgindent a run over the back > > branches. > > Yea, that thought has crossed our minds, but the problem is that there > is little testing of back branches so it would be risky. That's a fair point, though I wonder how can a code indenter be so broken that it broke code in ways not detected by our buildfarm. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera wrote: > Tom Lane wrote: > > Bruce Momjian <[EMAIL PROTECTED]> writes: > > > > I hate to say it but pgindent formatting changes are usually made in > > > cases where you or the community want pgindent to improve its indenting. > > > :-) > > > So we improve pgindent but pay for it in backpatching difficulties. :-( > > > > Yeah, I know ... it's why I've pretty much stopped complaining about > > pgindent, even though there are lots of little things it does that > > I don't especially care for. > > Maybe this means that we should give pgindent a run over the back > branches. Yea, that thought has crossed our minds, but the problem is that there is little testing of back branches so it would be risky. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > I hate to say it but pgindent formatting changes are usually made in > > cases where you or the community want pgindent to improve its indenting. :-) > > So we improve pgindent but pay for it in backpatching difficulties. :-( > > Yeah, I know ... it's why I've pretty much stopped complaining about > pgindent, even though there are lots of little things it does that > I don't especially care for. Maybe this means that we should give pgindent a run over the back branches. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> It's bad enough that Bruce whacks around his copy from time to time :-(. > I hate to say it but pgindent formatting changes are usually made in > cases where you or the community want pgindent to improve its indenting. :-) > So we improve pgindent but pay for it in backpatching difficulties. :-( Yeah, I know ... it's why I've pretty much stopped complaining about pgindent, even though there are lots of little things it does that I don't especially care for. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote:
> The main problem I see with "pgindent early and often" is that it only
> works well if everyone is using exactly the same pgindent code (and
> exactly the same typedef list). Otherwise you just get buried in
> useless whitespace diffs.
>
> It's bad enough that Bruce whacks around his copy from time to time :-(.
> I would say that the single greatest annoyance for maintaining our back
> branches is that patches tend to not back-patch cleanly, and well over
> half the time it's because of random reformatting done by pgindent
> to code that hadn't changed at all, but it had formatted differently
> the prior year.
I hate to say it but pgindent formatting changes are usually made in
cases where you or the community want pgindent to improve its indenting. :-)
So we improve pgindent but pay for it in backpatching difficulties. :-(
> I guess an advantage of maintaining our own fork is that there'd be Only
> One True pgindent, thereby alleviating that problem; but I'm still not
> eager to go there. If we were going to do it, I'd really wish that we
> could standardize on a version that didn't need a typedef list. The
I don't think that is possible. GNU indent 2.2.9 requires the same -T
option:
You must use the -T option to tell indent the name of all the
type names in your program that are defined by typedef. -T can be
specified more than once, and all names specified are used. For
example, if your program contains
typedef unsigned long CODE_ADDR;
typedef enum {red, blue, green} COLOR;
you would use the options -T CODE_ADDR -T COLOR
astyle doesn't seem to require it but perhaps it just mishandles them.
As I remember there isn't anything about the C grammar that can tell
identify a typedef when it needs to.
> But in any case, this is all focusing on trivialities. The stuff
> pgindent can fix is, by definition, stuff that committers don't really
> have to worry about during patch review. The stuff I'm worried about
> is at a higher level than that.
Agreed. Reformatting is small compared to understanding the patch,
adjusting in the comments, testing, documentation, etc.
--
Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us
EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Chris Browne <[EMAIL PROTECTED]> writes: > Would it be a terrible idea to... > > - Draw the indent code from NetBSD into src/tools/pgindent I am not real eager to become maintainers of our own indent fork, which is what you propose. (Just for starters, what will we have to do to make it run on non-BSD systems?) > We are presently at the extreme position where pgindent is run once in > a very long time (~ once a year), at pretty considerable cost, and > with the associated cost that a whole lot of indentation problems are > managed by hand. Yeah. One reason for that is that the typedef problem makes it a pretty manual process. The main problem I see with "pgindent early and often" is that it only works well if everyone is using exactly the same pgindent code (and exactly the same typedef list). Otherwise you just get buried in useless whitespace diffs. It's bad enough that Bruce whacks around his copy from time to time :-(. I would say that the single greatest annoyance for maintaining our back branches is that patches tend to not back-patch cleanly, and well over half the time it's because of random reformattings done by pgindent to code that hadn't changed at all, but it had formatted differently the prior year. For the same reason, my take on your "random whitespace changes are acceptable" theory is not no but hell no. It's gonna cost us, permanently, in manual patch adjustments if we allow the repository to get cluttered with content-free diffs. I guess an advantage of maintaining our own fork is that there'd be Only One True pgindent, thereby alleviating that problem; but I'm still not eager to go there. If we were going to do it, I'd really wish that we could standardize on a version that didn't need a typedef list. The random whitespace changes you get after changing the typedef list are another thorn in my side. But in any case, this is all focusing on trivialities. The stuff pgindent can fix is, by definition, stuff that committers don't really have to worry about during patch review. The stuff I'm worried about is at a higher level than that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Chris Browne wrote: > >> Would it be a terrible idea to... > >> > >> - Draw the indent code from NetBSD into src/tools/pgindent > >> - Build it _in place_ inside the code tree (e.g. - don't assume > >> it will get installed in /usr/local/bin) > >> - Thus have the ability to run it in place? > > > > Yes, but it bloats our code and people still need to generate the > > typedefs and follow the instructions. The other problem is if they run > > it on a file they have modified, it is going to adjust places they > > didn't touch, thereby making the patch harder to review. > > The bloat is 154K, on a project with something around 260MB of code. > I don't think this is a particlarly material degree of bloat. You mean 37kb vs 13MB, right? That is the tarball sizes I see. > If we ran pgindent really frequently, there would admittedly be the > difference that there would be a lot of little cases of > changes-from-pgindent being committed along the way, but [1] might it > not be cheaper to accept that cost, with the concomittant benefit that > you could tell patchers "Hey, run pgindent before submitting that > patch, and that'll clean up a number of of the issues." Yes, it > doesn't address code changes like typedef generation, but that never > was an argument against running pgindent... That is much a more radical use of pgindent than it has had in the past but it is certainly possible. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
[EMAIL PROTECTED] (Bruce Momjian) writes: > Chris Browne wrote: >> [EMAIL PROTECTED] (Bruce Momjian) writes: >> > Magnus Hagander wrote: >> >> > And I think adopting surrounding naming, commeting, coding conventions >> >> > should come naturally as it can aide in copy-pasting too :) >> >> >> >> I think pg_indent has to be made a lot more portable and easy to use >> >> before that can happen :-) I've run it once or twice on linux machines, >> >> and it comes out with huge changes compared to what Bruce gets on his >> >> machine. Other times, it doesn't :-) So yeah, it could be that it just >> >> needs to be made easier to use, because I may certainly have done >> >> something wrong. >> > >> > Agreed, pgindent is too cumbersome to require patch submitters to use. >> > One idea would be to allow C files to be emailed and the indented >> > version automatically returned via email. >> >> Would it be a terrible idea to... >> >> - Draw the indent code from NetBSD into src/tools/pgindent >> - Build it _in place_ inside the code tree (e.g. - don't assume >> it will get installed in /usr/local/bin) >> - Thus have the ability to run it in place? > > Yes, but it bloats our code and people still need to generate the > typedefs and follow the instructions. The other problem is if they run > it on a file they have modified, it is going to adjust places they > didn't touch, thereby making the patch harder to review. The bloat is 154K, on a project with something around 260MB of code. I don't think this is a particlarly material degree of bloat. If it is included in src/tools/pgindent, you can add in a Makefile such that it is automatically built, so the cost of running it goes way down, so that it could be run all the time rather than once in a great long while. If it was being run *all* the time, would we not expect to find that we would be seeing relatively smaller sets of changes coming out of it? We are presently at the extreme position where pgindent is run once in a very long time (~ once a year), at pretty considerable cost, and with the associated cost that a whole lot of indentation problems are managed by hand. If we ran pgindent really frequently, there would admittedly be the difference that there would be a lot of little cases of changes-from-pgindent being committed along the way, but [1] might it not be cheaper to accept that cost, with the concomittant benefit that you could tell patchers "Hey, run pgindent before submitting that patch, and that'll clean up a number of of the issues." Yes, it doesn't address code changes like typedef generation, but that never was an argument against running pgindent... [1] In cases where the differences primarily fall from differences in indentation, "cvs diff -u" can drop out those differences when reading the effects of a patch... -- let name="cbbrowne" and tld="cbbrowne.com" in name ^ "@" ^ tld;; http://linuxdatabases.info/info/sap.html "A study in the Washington Post says that women have better verbal skills than men. I just want to say to the authors of that study: Duh." -- Conan O' Brien -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Bruce Momjian wrote: > Chris Browne wrote: > > Would it be a terrible idea to... > > > > - Draw the indent code from NetBSD into src/tools/pgindent > > - Build it _in place_ inside the code tree (e.g. - don't assume > > it will get installed in /usr/local/bin) > > - Thus have the ability to run it in place? > > Yes, but it bloats our code and people still need to generate the > typedefs and follow the instructions. I suggested to you awhile back that we could keep a typedef file on the repository, saving one step. I don't see the problem with keeping the BSD indent for now, until we figure out the set of options GNU indent needs to behave properly. It's not all that much code, after all. > The other problem is if they run it on a file they have modified, it > is going to adjust places they didn't touch, thereby making the patch > harder to review. That should be rare if pgindent is run frequently, I think. (And anyway, there are tools to remove unwanted hunks from patches if the author so desires.) I guess the point here is that pgindent is a tool that should _help_ developers. Making it harder to run is not helping anyone. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Chris Browne wrote: > [EMAIL PROTECTED] (Bruce Momjian) writes: > > Magnus Hagander wrote: > >> > And I think adopting surrounding naming, commeting, coding conventions > >> > should come naturally as it can aide in copy-pasting too :) > >> > >> I think pg_indent has to be made a lot more portable and easy to use > >> before that can happen :-) I've run it once or twice on linux machines, > >> and it comes out with huge changes compared to what Bruce gets on his > >> machine. Other times, it doesn't :-) So yeah, it could be that it just > >> needs to be made easier to use, because I may certainly have done > >> something wrong. > > > > Agreed, pgindent is too cumbersome to require patch submitters to use. > > One idea would be to allow C files to be emailed and the indented > > version automatically returned via email. > > Would it be a terrible idea to... > > - Draw the indent code from NetBSD into src/tools/pgindent > - Build it _in place_ inside the code tree (e.g. - don't assume > it will get installed in /usr/local/bin) > - Thus have the ability to run it in place? Yes, but it bloats our code and people still need to generate the typedefs and follow the instructions. The other problem is if they run it on a file they have modified, it is going to adjust places they didn't touch, thereby making the patch harder to review. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
[EMAIL PROTECTED] (Bruce Momjian) writes: > Magnus Hagander wrote: >> > And I think adopting surrounding naming, commeting, coding conventions >> > should come naturally as it can aide in copy-pasting too :) >> >> I think pg_indent has to be made a lot more portable and easy to use >> before that can happen :-) I've run it once or twice on linux machines, >> and it comes out with huge changes compared to what Bruce gets on his >> machine. Other times, it doesn't :-) So yeah, it could be that it just >> needs to be made easier to use, because I may certainly have done >> something wrong. > > Agreed, pgindent is too cumbersome to require patch submitters to use. > One idea would be to allow C files to be emailed and the indented > version automatically returned via email. Would it be a terrible idea to... - Draw the indent code from NetBSD into src/tools/pgindent - Build it _in place_ inside the code tree (e.g. - don't assume it will get installed in /usr/local/bin) - Thus have the ability to run it in place? -- let name="cbbrowne" and tld="linuxfinances.info" in name ^ "@" ^ tld;; http://cbbrowne.com/info/lisp.html "It worked about as well as sticking a blender in the middle of a lime plantation and hoping they'll make margaritas out of themselves." -- Frederick J. Polsky v1.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Chris Browne wrote: > [EMAIL PROTECTED] (Tom Lane) writes: > > Magnus Hagander <[EMAIL PROTECTED]> writes: > >> I think pg_indent has to be made a lot more portable and easy to use > >> before that can happen :-) I've run it once or twice on linux machines, > >> and it comes out with huge changes compared to what Bruce gets on his > >> machine. > > > > Yeah, I've had no luck with it either. > > > > Every so often there are discussions of going over to GNU indent > > instead. Presumably that would solve the portability problem. > > The last time we tried it (which was a long time ago) it seemed > > to have too many bugs and idiosyncrasies of its own, but it would > > be worth a fresh round of experimenting IMHO. > > Well, GNU indent is now on version 2.2.9, and has evidently addressed > *some* problems with it. > > Unfortunately, the pgindent README does not actually specify what any > of the actual problems with GNU indent are, thus making it pretty much > impossible to evaluate whether or not any of the subsequent releases > might have addressed any of those problems. > > I doubt that the pgindent issues have been addressed. What I did was to run pgindent on the source tree, make a copy, then run GNU indent on it and diff -r. I can try that test again in the next few months. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
On Wed, 16 Apr 2008 12:36:50 -0400 (EDT) Bruce Momjian <[EMAIL PROTECTED]> wrote: > > If the patch submitters hasn't read the developers' FAQ, we assume > they are not interested in improving the style of their patch. I think that point is fairly flawed in consideration. I know for a fact that I have had to repeat even to long time contributors source references for information even though they new about it previously. Yourself included. People get in a hurry, they should be reminded with reference. JD, thanks for the patch but you missed foo.. please check 1.5 of the Developers FAQ. Sincerely, Joshua D. Drake -- The PostgreSQL Company since 1997: http://www.commandprompt.com/ PostgreSQL Community Conference: http://www.postgresqlconference.org/ United States PostgreSQL Association: http://www.postgresql.us/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Chris Browne <[EMAIL PROTECTED]> writes: > [EMAIL PROTECTED] (Tom Lane) writes: >> Every so often there are discussions of going over to GNU indent >> instead. Presumably that would solve the portability problem. >> The last time we tried it (which was a long time ago) it seemed >> to have too many bugs and idiosyncrasies of its own, but it would >> be worth a fresh round of experimenting IMHO. > Well, GNU indent is now on version 2.2.9, and has evidently addressed > *some* problems with it. > Unfortunately, the pgindent README does not actually specify what any > of the actual problems with GNU indent are, thus making it pretty much > impossible to evaluate whether or not any of the subsequent releases > might have addressed any of those problems. This wouldn't be hard for someone to investigate. Check out our CVS from immediately after the last pgindent run. Run GNU indent on it. See what options result in the smallest diff, and what the diff looks like in terms of making the code look nicer or less nice. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
[EMAIL PROTECTED] (Tom Lane) writes:
> Magnus Hagander <[EMAIL PROTECTED]> writes:
>> I think pg_indent has to be made a lot more portable and easy to use
>> before that can happen :-) I've run it once or twice on linux machines,
>> and it comes out with huge changes compared to what Bruce gets on his
>> machine.
>
> Yeah, I've had no luck with it either.
>
> Every so often there are discussions of going over to GNU indent
> instead. Presumably that would solve the portability problem.
> The last time we tried it (which was a long time ago) it seemed
> to have too many bugs and idiosyncrasies of its own, but it would
> be worth a fresh round of experimenting IMHO.
Well, GNU indent is now on version 2.2.9, and has evidently addressed
*some* problems with it.
Unfortunately, the pgindent README does not actually specify what any
of the actual problems with GNU indent are, thus making it pretty much
impossible to evaluate whether or not any of the subsequent releases
might have addressed any of those problems.
I doubt that the pgindent issues have been addressed.
--
output = reverse("ofni.sesabatadxunil" "@" "enworbbc")
http://linuxfinances.info/info/sgml.html
"In elementary school, in case of fire you have to line up quietly in
a single file line from smallest to tallest. What is the logic? Do
tall people burn slower?" -- Warren Hutcherson
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Magnus Hagander wrote: > > And I think adopting surrounding naming, commeting, coding conventions > > should come naturally as it can aide in copy-pasting too :) > > I think pg_indent has to be made a lot more portable and easy to use > before that can happen :-) I've run it once or twice on linux machines, > and it comes out with huge changes compared to what Bruce gets on his > machine. Other times, it doesn't :-) So yeah, it could be that it just > needs to be made easier to use, because I may certainly have done > something wrong. Agreed, pgindent is too cumbersome to require patch submitters to use. One idea would be to allow C files to be emailed and the indented version automatically returned via email. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
Brendan Jurd wrote: > > The idea that we "fix" stylistic issues on the fly is not sustainable. > > We should offer help and mentorship to new patch submitters in all > > areas (including stylistic) but they should do the work. It is the only > > way we will mold them to submit patches in the proper way. > > > > I agree. As a submitter I would much rather get an email saying e.g. > "Hey, your patch is nice but the code style sticks out like a sore > thumb. Please adopt surrounding naming convention and fix your > indentation per the rules at [link]." than have it fixed silently on > its way to being committed. > > With the former I learn something and get to improve my own work. > With the latter, my next patch is probably going to have the exact > same problem, which is in the long term just making extra work for the > reviewers. If the patch submitters hasn't read the developers' FAQ, we assume they are not interested in improving the style of their patch. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://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
Re: [HACKERS] Lessons from commit fest
On Wed, Apr 16, 2008 at 1:07 PM, Magnus Hagander <[EMAIL PROTECTED]> wrote: > I think pg_indent has to be made a lot more portable and easy to use > before that can happen :-) I've run it once or twice on linux machines, > and it comes out with huge changes compared to what Bruce gets on his > machine. Other times, it doesn't :-) So yeah, it could be that it just > needs to be made easier to use, because I may certainly have done > something wrong. Yeah, I recall trying it a while ago and not having happy memories. It should be possible (and indeed, mandatory) for patch submitters to run it over their code before submitting it - having core guys whose time is so valuable dealing with indentation etc seems an incredible waste. It won't fix everything, as Tom points out, but it's a better starting point. Cheers Tom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Magnus Hagander <[EMAIL PROTECTED]> writes: > I think pg_indent has to be made a lot more portable and easy to use > before that can happen :-) I've run it once or twice on linux machines, > and it comes out with huge changes compared to what Bruce gets on his > machine. Yeah, I've had no luck with it either. Every so often there are discussions of going over to GNU indent instead. Presumably that would solve the portability problem. The last time we tried it (which was a long time ago) it seemed to have too many bugs and idiosyncrasies of its own, but it would be worth a fresh round of experimenting IMHO. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
NikhilS wrote: > Hi, > > > The idea that we "fix" stylistic issues on the fly is not > > sustainable. > > > We should offer help and mentorship to new patch submitters in > > > all areas (including stylistic) but they should do the work. It > > > is the only way we will mold them to submit patches in the proper > > > way. > > > > > > > I agree. As a submitter I would much rather get an email saying > > e.g. "Hey, your patch is nice but the code style sticks out like a > > sore thumb. Please adopt surrounding naming convention and fix your > > indentation per the rules at [link]." than have it fixed silently on > > its way to being committed. > > > > With the former I learn something and get to improve my own work. > > With the latter, my next patch is probably going to have the exact > > same problem, which is in the long term just making extra work for > > the reviewers. > > > > I think, us patch-submitters should be asked to do a run of pg_indent > on the files that we have modified. That should take care of atleast > the indentation related issues. I looked at the README of > src/tools/pgindent, and it should be easy to run enough (or is it > not?). Only one thing that caught my eye was: > > 1) Build the source tree with _debug_ symbols and all possible > configure options > > Can the above point be elaborated further? What all typical and > possible configure options should be used to get a clean and complete > pg_indent run? > > And I think adopting surrounding naming, commeting, coding conventions > should come naturally as it can aide in copy-pasting too :) I think pg_indent has to be made a lot more portable and easy to use before that can happen :-) I've run it once or twice on linux machines, and it comes out with huge changes compared to what Bruce gets on his machine. Other times, it doesn't :-) So yeah, it could be that it just needs to be made easier to use, because I may certainly have done something wrong. //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Hi, > The idea that we "fix" stylistic issues on the fly is not sustainable. > > We should offer help and mentorship to new patch submitters in all > > areas (including stylistic) but they should do the work. It is the only > > way we will mold them to submit patches in the proper way. > > > > I agree. As a submitter I would much rather get an email saying e.g. > "Hey, your patch is nice but the code style sticks out like a sore > thumb. Please adopt surrounding naming convention and fix your > indentation per the rules at [link]." than have it fixed silently on > its way to being committed. > > With the former I learn something and get to improve my own work. > With the latter, my next patch is probably going to have the exact > same problem, which is in the long term just making extra work for the > reviewers. > I think, us patch-submitters should be asked to do a run of pg_indent on the files that we have modified. That should take care of atleast the indentation related issues. I looked at the README of src/tools/pgindent, and it should be easy to run enough (or is it not?). Only one thing that caught my eye was: 1) Build the source tree with _debug_ symbols and all possible configure options Can the above point be elaborated further? What all typical and possible configure options should be used to get a clean and complete pg_indent run? And I think adopting surrounding naming, commeting, coding conventions should come naturally as it can aide in copy-pasting too :) Regards, Nikhils -- EnterpriseDB http://www.enterprisedb.com
Re: [HACKERS] Lessons from commit fest
On Apr 14, 2008, at 4:15 PM, Bruce Momjian wrote: If you don't want an issue to get forgotten, then make a TODO entry for it. But the purpose of commit fest is to make sure we deal with things that can be dealt with in a timely fashion. It's not going to cause solutions to unsolved problems to appear from nowhere. I need is to know if they are ideas worthy of TODO. This is something I think we could really improve upon... For one, I think the impression (right or wrong) is that Bruce is the person that gets say on what goes on the TODO. We also don't have any good mechanism for general users to provide feedback on what things are important to them. Of course it's ultimately about a developer scratching an itch, but I think it would be useful to have some idea of how popular TODO items were to help guide development efforts, as well as possibly target items that should just be removed. -- Decibel!, aka Jim C. Nasby, Database Architect [EMAIL PROTECTED] Give your computer some brain candy! www.distributed.net Team #1828 smime.p7s Description: S/MIME cryptographic signature
Re: [HACKERS] Lessons from commit fest
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Wed, Apr 16, 2008 at 2:17 AM, Joshua D. Drake wrote: > On Tue, 15 Apr 2008 12:12:24 -0400 > Alvaro Herrera wrote: > > > Tom Lane wrote: > > > > > I tend to just fix this stuff while committing, rather than lecture > > > the submitters about it, but it undoubtedly is a time sink. > > > > Lesson learned: a useful task for another reviewer to do is to grab > > the patch, fix the style issues, and post the fixed version. That > > way, the "higher level reviewer" does not have to waste time on a > > task that, really, anybody can do. > > The idea that we "fix" stylistic issues on the fly is not sustainable. > We should offer help and mentorship to new patch submitters in all > areas (including stylistic) but they should do the work. It is the only > way we will mold them to submit patches in the proper way. > I agree. As a submitter I would much rather get an email saying e.g. "Hey, your patch is nice but the code style sticks out like a sore thumb. Please adopt surrounding naming convention and fix your indentation per the rules at [link]." than have it fixed silently on its way to being committed. With the former I learn something and get to improve my own work. With the latter, my next patch is probably going to have the exact same problem, which is in the long term just making extra work for the reviewers. Cheers, BJ -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.7 (GNU/Linux) Comment: http://getfiregpg.org iD8DBQFIBZZ95YBsbHkuyV0RAjviAJ9pBG6I3DAySIx6ARp2cLnYe+bAdACg/VAR Xgvpg4iyWsbNk4QKGI//diw= =yDoO -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
On Tue, 15 Apr 2008 12:12:24 -0400 Alvaro Herrera <[EMAIL PROTECTED]> wrote: > Tom Lane wrote: > > > I tend to just fix this stuff while committing, rather than lecture > > the submitters about it, but it undoubtedly is a time sink. > > Lesson learned: a useful task for another reviewer to do is to grab > the patch, fix the style issues, and post the fixed version. That > way, the "higher level reviewer" does not have to waste time on a > task that, really, anybody can do. This reminds me of parenting. As a parent you have a tendency to do things for your children, long past the time you should. Maybe it is that you cut their food so they don't have to use a knife or that you wash their face with their sleeve as they walk out the door. At some point you have to let go otherwise the child will never learn to take care of themselves and will end up living in your basement, unemployed and yelling up the stairs for bagel bytes. The idea that we "fix" stylistic issues on the fly is not sustainable. We should offer help and mentorship to new patch submitters in all areas (including stylistic) but they should do the work. It is the only way we will mold them to submit patches in the proper way. Sincerely, Joshua D. Drake -- The PostgreSQL Company since 1997: http://www.commandprompt.com/ PostgreSQL Community Conference: http://www.postgresqlconference.org/ United States PostgreSQL Association: http://www.postgresql.us/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > I tend to just fix this stuff while committing, rather than lecture the > submitters about it, but it undoubtedly is a time sink. Lesson learned: a useful task for another reviewer to do is to grab the patch, fix the style issues, and post the fixed version. That way, the "higher level reviewer" does not have to waste time on a task that, really, anybody can do. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> A general comment is that in stuff I review, I frequently spend a lot of >> time trying to make the patch "look like it belongs", that is make it >> reasonably well-integrated with the surrounding code. This is important >> because a code base that too obviously consists of layers upon layers >> of independent patches soon ceases to be readable or maintainable. > I did waste some time in the past complaining to submitters when the > style was off. At some point I stopped because I got the impression > that that style of comment was not useful: people seem to get the idea > that it's OK if the code does not follow our style; pgindent would fix > it later after all. pg_indent can fix some things, but a lot of what I'm thinking about here is far beyond its ken. You also have to be aware that it can mangle comments to the point of unreadability, if the comment style is not what it expects --- even relatively simple things like using two stars instead of one for block comment leader can look pretty ugly after pg_indent. I tend to just fix this stuff while committing, rather than lecture the submitters about it, but it undoubtedly is a time sink. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Tom Lane wrote: > A general comment is that in stuff I review, I frequently spend a lot of > time trying to make the patch "look like it belongs", that is make it > reasonably well-integrated with the surrounding code. This is important > because a code base that too obviously consists of layers upon layers > of independent patches soon ceases to be readable or maintainable. I did waste some time in the past complaining to submitters when the style was off. At some point I stopped because I got the impression that that style of comment was not useful: people seem to get the idea that it's OK if the code does not follow our style; pgindent would fix it later after all. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Gregory Stark <[EMAIL PROTECTED]> writes: > I don't think we know what a "typical review" really looks like. A general comment is that in stuff I review, I frequently spend a lot of time trying to make the patch "look like it belongs", that is make it reasonably well-integrated with the surrounding code. This is important because a code base that too obviously consists of layers upon layers of independent patches soon ceases to be readable or maintainable. Ideally, once your patch has gone in, someone looking at the code for the first time wouldn't even suspect it hadn't always been there. Typical sins in this area include not following the coding style or commenting style of immediately adjacent code, failing to update comments that your patch has rendered inaccurate, intentionally making your edits "stand out", etc. While pg_indent will fix some of the simpler transgressions, it's not very good with comment style, and it certainly can't fix failures of omission. (I dislike committing code that is far away from pg_indent style anyway, since even if it will get fixed later, I'm still gonna have to look at it until then.) Sometimes patch authors seem to prefer shorter patches over better integrated ones --- this particularly happens when the "most natural" way of adding something would require refactoring existing code. I understand the reasons for preferring a smaller patch, but you really need to take the long view about what the code will look like later. I guess this is coming off as more advice to patch authors than reviewers, but it is definitely a big deal in my eyes --- I typically spend about as much time on issues of this sort as on functional correctness. And it is something that reviewers can fix if they care to. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Gregory Stark <[EMAIL PROTECTED]> writes: > One thing I found is that I'm not sure what to do if I don't find any changes > to propose. I tend to assume that means I just don't understand the code well > enough and end up just not posting anything. It's still worth adding an annotation to the wiki that you looked it over and couldn't find anything wrong. A couple such annotations would give the eventual committer a warm fuzzy feeling that he didn't need to look at the patch too hard. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
"Stephen Frost" <[EMAIL PROTECTED]> writes: > * Tom Lane ([EMAIL PROTECTED]) wrote: >> One problem with this fest was that a whole lot of the patches *weren't* >> trivial; if they had been they'd not have gotten put off till 8.4. So >> there weren't that many that I wanted to let go in without looking at >> them. I guess that's just another way in which the 8.3 schedule problem >> came home to roost during this fest. > > That's an issue I ran into when looking through the patches as well. I > can help review trivial patches but I don't generally have the > bandwidth to review alot of the pretty heavy patches which were in the > March commitfest. It took me a while to get going on the commitfest too > though, in part because I wasn't really confident I knew the right > places to look and the right things to do. I'm still not 100% sure, > honestly. Do we have guidelines up somewhere about reviewing, specific > to the process rather than about how to submit patches? I don't think we know what a "typical review" really looks like. But basically what worked for me in the (relatively trivial) patches I've looked at was considering "if this was mine, what would I consider doing next?" I made a bullet point list of things I would consider changing or think are missing. One thing I found is that I'm not sure what to do if I don't find any changes to propose. I tend to assume that means I just don't understand the code well enough and end up just not posting anything. >> Perhaps it would be useful to try to rate pending patches by difficulty? > > I think alot of times this can be determined pretty quickly, and I'd > hate to have a situation where patch submitters are complaining about > "you rated my patch harder than his and that's not fair" kind of things. One thing which is conventional on linux-kernel is to include the output of diffstat when you post the patch. That helps people to get an idea of whether their favourite area of the code is being whacked around and it warrants a look. It also helps get a quick overview of what to expect as you start to read the patch proper. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
Martijn van Oosterhout napsal(a): On Mon, Apr 14, 2008 at 05:15:51PM -0400, Bruce Momjian wrote: So when/how do those discussions get resolved? [ shrug... ] You can't force ideas to happen on a schedule. I need is to know if they are ideas worthy of TODO. One thing I would like is if larger more complex patches where there really was discussion would get a page in the wiki. For example the FSM data structure discussion; I read the thread but due to all the crossing threads I only have a have vague idea of what's actually been decided. It would be helpful if there was a wiki page that described the solution and why it is better than the others suggested. +1 Completely agree. Each huge page should have actual proposal/design on wiki. It helps to improve speed of review and also it will be good knowledge base about internals. Zdenek -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Lessons from commit fest
* Tom Lane ([EMAIL PROTECTED]) wrote: > One problem with this fest was that a whole lot of the patches *weren't* > trivial; if they had been they'd not have gotten put off till 8.4. So > there weren't that many that I wanted to let go in without looking at > them. I guess that's just another way in which the 8.3 schedule problem > came home to roost during this fest. That's an issue I ran into when looking through the patches as well. I can help review trivial patches but I don't generally have the bandwidth to review alot of the pretty heavy patches which were in the March commitfest. It took me a while to get going on the commitfest too though, in part because I wasn't really confident I knew the right places to look and the right things to do. I'm still not 100% sure, honestly. Do we have guidelines up somewhere about reviewing, specific to the process rather than about how to submit patches? > In future fests we should have a much higher proportion of "little > things" that maybe more people would feel comfortable taking > responsibility for. I'm certainly happy to look at little things and review those and at least add my own comments. I think that was helpful for the patches I did comment on in the past, though if not please let me know. > Perhaps it would be useful to try to rate pending patches by difficulty? I think alot of times this can be determined pretty quickly, and I'd hate to have a situation where patch submitters are complaining about "you rated my patch harder than his and that's not fair" kind of things. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Lessons from commit fest
On Mon, Apr 14, 2008 at 05:15:51PM -0400, Bruce Momjian wrote: > > > So when/how do those discussions get resolved? > > > > [ shrug... ] You can't force ideas to happen on a schedule. > > I need is to know if they are ideas worthy of TODO. One thing I would like is if larger more complex patches where there really was discussion would get a page in the wiki. For example the FSM data structure discussion; I read the thread but due to all the crossing threads I only have a have vague idea of what's actually been decided. It would be helpful if there was a wiki page that described the solution and why it is better than the others suggested. Have a nice day, -- Martijn van Oosterhout <[EMAIL PROTECTED]> http://svana.org/kleptog/ > Please line up in a tree and maintain the heap invariant while > boarding. Thank you for flying nlogn airlines. signature.asc Description: Digital signature
Re: [HACKERS] Lessons from commit fest
On Mon, 14 Apr 2008 21:25:28 -0400 Alvaro Herrera <[EMAIL PROTECTED]> wrote: > The problem is that the patch was initially trivial, but it turned > into a much larger redesign of command handling. I think that's a > great turnout for a submission. > > > Don't forget to update developer FAQ as well. :) > > Agreed -- the FAQs and other documents do not cover the processes > we're currently following. Mind you, the processes are quite young. > (More reason to have them better documented I guess.) We can change the FAQ per commit fest as things grow and as each commit fest starts, send out the policy for the commit fest on announce. Joshua D. Drake -- The PostgreSQL Company since 1997: http://www.commandprompt.com/ PostgreSQL Community Conference: http://www.postgresqlconference.org/ United States PostgreSQL Association: http://www.postgresql.us/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
