Thread: Keyword list sanity check
I wrote a little perl script to perform a basic sanity check to keywords in gram.y and kwlist.h. It checks that all lists are in alphabetical order, all keywords present in gram.y are listed in kwlist.h in the right category, and conversely that all keywords listed in kwlist.h are listed in gram.y. It found one minor issue already: $ perl src/tools/check_keywords.pl 'SCHEMA' after 'SERVER' in unreserved_keyword list is misplaced SERVER is not in the right place in gram.y, it should go between SERIALIZABLE and SERVER. I'll fix that. I'll put this into src/tools. It's heavily dependent on the format of the lists in gram.y and kwlist.h but if it bitrots due to changes in those files, we can either fix it or just remove it if it's not deemed useful anymore. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Attachment
I had previously considered adding an assertion in the backend to check they're sorted properly. That would be less formatting dependent and would be only a couple lines of C. I don't think we can do that with the gram.y check though. -- Greg On 28 Apr 2009, at 09:33, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com > wrote: > I wrote a little perl script to perform a basic sanity check to > keywords in gram.y and kwlist.h. It checks that all lists are in > alphabetical order, all keywords present in gram.y are listed in > kwlist.h in the right category, and conversely that all keywords > listed in kwlist.h are listed in gram.y. > > It found one minor issue already: > > $ perl src/tools/check_keywords.pl > 'SCHEMA' after 'SERVER' in unreserved_keyword list is misplaced > > SERVER is not in the right place in gram.y, it should go between > SERIALIZABLE and SERVER. I'll fix that. > > I'll put this into src/tools. It's heavily dependent on the format > of the lists in gram.y and kwlist.h but if it bitrots due to changes > in those files, we can either fix it or just remove it if it's not > deemed useful anymore. > > -- > Heikki Linnakangas > EnterpriseDB http://www.enterprisedb.com > <check_keywords.pl> > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
Greg Stark wrote: > I had previously considered adding an assertion in the backend to check > they're sorted properly. That would be less formatting dependent and > would be only a couple lines of C. > > I don't think we can do that with the gram.y check though. Well, the ordering in gram.y is just pro forma anyway. Checking that all keywords are present and correctly labeled in kwlist.h is more important. What's still missing is to check that all keywords defined with "%token <keyword>" in gram.y are present in one of the keyword lists. Hmm, it just occurred to me that this script is only a few steps away from actually generating kwlist.h from gram.y, instead of merely cross-checking them. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Tue, Apr 28, 2009 at 11:57:19AM +0300, Heikki Linnakangas wrote: > Greg Stark wrote: >> I had previously considered adding an assertion in the backend to >> check they're sorted properly. That would be less formatting >> dependent and would be only a couple lines of C. >> >> I don't think we can do that with the gram.y check though. > > Well, the ordering in gram.y is just pro forma anyway. Checking that > all keywords are present and correctly labeled in kwlist.h is more > important. What's still missing is to check that all keywords > defined with "%token <keyword>" in gram.y are present in one of the > keyword lists. > > Hmm, it just occurred to me that this script is only a few steps > away from actually generating kwlist.h from gram.y, instead of > merely cross-checking them. Single Point of Truth is good :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Apr 28, 2009 at 11:33:28AM +0300, Heikki Linnakangas wrote: > I wrote a little perl script to perform a basic sanity check to > keywords in gram.y and kwlist.h. It checks that all lists are in > alphabetical order, all keywords present in gram.y are listed in > kwlist.h in the right category, and conversely that all keywords > listed in kwlist.h are listed in gram.y. > > It found one minor issue already: > > $ perl src/tools/check_keywords.pl 'SCHEMA' after 'SERVER' in > unreserved_keyword list is misplaced > > SERVER is not in the right place in gram.y, it should go between > SERIALIZABLE and SERVER. I'll fix that. > > I'll put this into src/tools. It's heavily dependent on the format > of the lists in gram.y and kwlist.h but if it bitrots due to > changes in those files, we can either fix it or just remove it if > it's not deemed useful anymore. Please clean up this code at least to the point where it's strict-clean, which means putting "use strict;" right after the shebang line and not checking it in until it runs that way. I tried, but couldn't make heads or tails of the thing, given all the unused- and similarly-named variables, failure to indent, etc. I don't know how to put this gently, but if you checked in C code with quality like this, you'd have it bounced with derision. I'd also like to propose that "strict clean" be a minimum code quality metric for any Perl code in our code base. A lot of what's in there is just about impossible to maintain. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Apr 30, 2009, at 2:27 AM, David Fetter wrote: > Please clean up this code at least to the point where it's > strict-clean, which means putting "use strict;" right after the > shebang line and not checking it in until it runs that way. I can take care of this, David. Shouldn't be too tough. -- Andy Lester => andy@petdance.com => www.petdance.com => AIM:petdance
On Thu, Apr 30, 2009 at 3:27 AM, David Fetter <david@fetter.org> wrote: > On Tue, Apr 28, 2009 at 11:33:28AM +0300, Heikki Linnakangas wrote: >> I wrote a little perl script to perform a basic sanity check to >> keywords in gram.y and kwlist.h. It checks that all lists are in >> alphabetical order, all keywords present in gram.y are listed in >> kwlist.h in the right category, and conversely that all keywords >> listed in kwlist.h are listed in gram.y. >> >> It found one minor issue already: >> >> $ perl src/tools/check_keywords.pl 'SCHEMA' after 'SERVER' in >> unreserved_keyword list is misplaced >> >> SERVER is not in the right place in gram.y, it should go between >> SERIALIZABLE and SERVER. I'll fix that. >> >> I'll put this into src/tools. It's heavily dependent on the format >> of the lists in gram.y and kwlist.h but if it bitrots due to >> changes in those files, we can either fix it or just remove it if >> it's not deemed useful anymore. > > Please clean up this code at least to the point where it's > strict-clean, which means putting "use strict;" right after the > shebang line and not checking it in until it runs that way. And "use warnings;", too. ...Robert
David Fetter wrote: > On Tue, Apr 28, 2009 at 11:33:28AM +0300, Heikki Linnakangas wrote: > > I wrote a little perl script to perform a basic sanity check to > > keywords in gram.y and kwlist.h. It checks that all lists are in > > alphabetical order, all keywords present in gram.y are listed in > > kwlist.h in the right category, and conversely that all keywords > > listed in kwlist.h are listed in gram.y. > > > > It found one minor issue already: > > > > $ perl src/tools/check_keywords.pl 'SCHEMA' after 'SERVER' in > > unreserved_keyword list is misplaced > > > > SERVER is not in the right place in gram.y, it should go between > > SERIALIZABLE and SERVER. I'll fix that. > > > > I'll put this into src/tools. It's heavily dependent on the format > > of the lists in gram.y and kwlist.h but if it bitrots due to > > changes in those files, we can either fix it or just remove it if > > it's not deemed useful anymore. > > Please clean up this code at least to the point where it's > strict-clean, which means putting "use strict;" right after the > shebang line and not checking it in until it runs that way. > > I tried, but couldn't make heads or tails of the thing, given all the > unused- and similarly-named variables, failure to indent, etc. I > don't know how to put this gently, but if you checked in C code with > quality like this, you'd have it bounced with derision. > > I'd also like to propose that "strict clean" be a minimum code quality > metric for any Perl code in our code base. A lot of what's in there > is just about impossible to maintain. Good suggestion; I see this has been done: revision 1.2date: 2009/04/30 10:26:35; author: heikki; state: Exp; lines: +24 -11Clean up check_keywords.pl script, makingit 'strict' and removing a fewleftover unused variables.Laurent Laborde -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Apr 30, 2009, at 6:41 AM, Robert Haas wrote: >> Please clean up this code at least to the point where it's >> strict-clean, which means putting "use strict;" right after the >> shebang line and not checking it in until it runs that way. > > And "use warnings;", too. I'll prob'ly come up with a policy file for Perl::Critic and a make target for perlcritic. xoa -- Andy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance
Robert Haas wrote: > On Thu, Apr 30, 2009 at 3:27 AM, David Fetter <david@fetter.org> wrote: > > On Tue, Apr 28, 2009 at 11:33:28AM +0300, Heikki Linnakangas wrote: > >> I wrote a little perl script to perform a basic sanity check to > >> keywords ?in gram.y and kwlist.h. It checks that all lists are in > >> alphabetical ?order, all keywords present in gram.y are listed in > >> kwlist.h in the ?right category, and conversely that all keywords > >> listed in kwlist.h are ?listed in gram.y. > >> > >> It found one minor issue already: > >> > >> $ perl src/tools/check_keywords.pl 'SCHEMA' after 'SERVER' in > >> unreserved_keyword list is misplaced > >> > >> SERVER is not in the right place in gram.y, it should go between > >> SERIALIZABLE and SERVER. I'll fix that. > >> > >> I'll put this into src/tools. It's heavily dependent on the format > >> of ?the lists in gram.y and kwlist.h but if it bitrots due to > >> changes in ?those files, we can either fix it or just remove it if > >> it's not deemed ?useful anymore. > > > > Please clean up this code at least to the point where it's > > strict-clean, which means putting "use strict;" right after the > > shebang line and not checking it in until it runs that way. > > And "use warnings;", too. I now see at the top of the CVS file: #!/usr/bin/perl -wuse strict; so it seems it has both now. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Thu, Apr 30, 2009 at 11:39:33AM -0500, Andy Lester wrote: > > On Apr 30, 2009, at 6:41 AM, Robert Haas wrote: > >>> Please clean up this code at least to the point where it's >>> strict-clean, which means putting "use strict;" right after the >>> shebang line and not checking it in until it runs that way. >> >> And "use warnings;", too. > > > I'll prob'ly come up with a policy file for Perl::Critic and a make > target for perlcritic. The current code has a bunch of 5s in it, so it's a target-rich environment :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Thursday 30 April 2009 10:27:45 David Fetter wrote: > I'd also like to propose that "strict clean" be a minimum code quality > metric for any Perl code in our code base. A lot of what's in there > is just about impossible to maintain. use strict and use warnings, I think, although with use warnings I have occasionally run into the trouble of some old versions not supporting it (only via perl -w).
Laurent Laborde wrote: > On Tue, Apr 28, 2009 at 10:33 AM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: >> I wrote a little perl script to perform a basic sanity check to keywords in >> gram.y and kwlist.h. It checks that all lists are in alphabetical order, all >> keywords present in gram.y are listed in kwlist.h in the right category, and >> conversely that all keywords listed in kwlist.h are listed in gram.y. > > Friendly greetings ! > Here is a new version of check_keywords.pl : > - perl -w and "use strict" enabled (and all the fixes that come with it) > - minor cleaning Thanks, applied! -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Thu, Apr 30, 2009 at 09:40:50AM -0700, David Fetter wrote: > On Thu, Apr 30, 2009 at 11:39:33AM -0500, Andy Lester wrote: > > > > On Apr 30, 2009, at 6:41 AM, Robert Haas wrote: > > > >>> Please clean up this code at least to the point where it's > >>> strict-clean, which means putting "use strict;" right after the > >>> shebang line and not checking it in until it runs that way. > >> > >> And "use warnings;", too. > > > > > > I'll prob'ly come up with a policy file for Perl::Critic and a make > > target for perlcritic. > > The current code has a bunch of 5s in it, so it's a target-rich > environment :) Here's a patch that gets it to pass perlcritic -4 and still (as far as I can tell) work. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachment
On Tue, Apr 28, 2009 at 10:33 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > I wrote a little perl script to perform a basic sanity check to keywords in > gram.y and kwlist.h. It checks that all lists are in alphabetical order, all > keywords present in gram.y are listed in kwlist.h in the right category, and > conversely that all keywords listed in kwlist.h are listed in gram.y. Friendly greetings ! Here is a new version of check_keywords.pl : - perl -w and "use strict" enabled (and all the fixes that come with it) - minor cleaning -- Laurent Laborde Sysadmin at jfg://networks http://www.over-blog.com/
Attachment
On Apr 30, 2009, at 2:11 PM, David Fetter wrote: > Here's a patch that gets it to pass perlcritic -4 and still (as far as > I can tell) work. Tell ya what. Let me at it and I'll give a larger, more inclusive patch. xoxo, Andy -- Andy Lester => andy@petdance.com => www.theworkinggeek.com => AIM:petdance
Peter Eisentraut wrote: > On Thursday 30 April 2009 10:27:45 David Fetter wrote: > >> I'd also like to propose that "strict clean" be a minimum code quality >> metric for any Perl code in our code base. A lot of what's in there >> is just about impossible to maintain. >> > > use strict and use warnings, I think, although with use warnings I have > occasionally run into the trouble of some old versions not supporting it (only > via perl -w). > > Right. I think strict mode is probably sufficient for utility code like this. Also note that we maintain the usual postgres indentation standards in the perl MSVC stuff, by using perltidy thus: perltidy -b -bl -nsfs -naws -l=100 -ole=unix *.pl *.pm cheers andrew
David Fetter wrote: > Please clean up this code at least to the point where it's > strict-clean, which means putting "use strict;" right after the > shebang line and not checking it in until it runs that way. > > I tried, but couldn't make heads or tails of the thing, given all the > unused- and similarly-named variables, failure to indent, etc. I > don't know how to put this gently, but if you checked in C code with > quality like this, you'd have it bounced with derision. Yep, I'm completely novice at perl. I've checked in Laurent's patch to clean it up, which at least makes it strict-clean. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com