Erroring out on parser conflicts

Peter Eisentraut <peter_e@gmx.net> writes:

While hacking on parser/gram.y just now I noticed in passing that the
automatically generated ecpg parser had 402 shift/reduce conflicts.
(Don't panic, the parser in CVS is fine.) If you don't pay very close
attention, it is easy to miss this. Considering also that we frequently
have to educate contributors that parser conflicts are not acceptable,
should we try to error out if we see conflicts?

Would "%expect 0" produce the same result in a less klugy way?

regards, tom lane

On Tuesday 25 November 2008 15:09:37 Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

While hacking on parser/gram.y just now I noticed in passing that the
automatically generated ecpg parser had 402 shift/reduce conflicts.
(Don't panic, the parser in CVS is fine.) If you don't pay very close
attention, it is easy to miss this. Considering also that we frequently
have to educate contributors that parser conflicts are not acceptable,
should we try to error out if we see conflicts?

Would "%expect 0" produce the same result in a less klugy way?

Great, that works. I'll see about adding this to our parser files.

Peter Eisentraut wrote:

On Tuesday 25 November 2008 15:09:37 Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

While hacking on parser/gram.y just now I noticed in passing that the
automatically generated ecpg parser had 402 shift/reduce conflicts.
(Don't panic, the parser in CVS is fine.) If you don't pay very close
attention, it is easy to miss this. Considering also that we frequently
have to educate contributors that parser conflicts are not acceptable,
should we try to error out if we see conflicts?

Would "%expect 0" produce the same result in a less klugy way?

Great, that works. I'll see about adding this to our parser files.

FYI, this is going to make it hard for developers to test CVS changes
until they get their grammar cleaned up; perhaps add a comment on how
to disable the check?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

FYI, this is going to make it hard for developers to test CVS changes
until they get their grammar cleaned up; perhaps add a comment on how
to disable the check?

Well, the point is that their grammar changes are broken if that check
fails, so I'm not sure what the value of "testing" a known-incorrect
grammar might be. It wouldn't necessarily act the same after being
fixed.

regards, tom lane

On 3 Dec 2008, at 03:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Bruce Momjian <bruce@momjian.us> writes:

FYI, this is going to make it hard for developers to test CVS changes
until they get their grammar cleaned up; perhaps add a comment on
how
to disable the check?

Well, the point is that their grammar changes are broken if that check
fails, so I'm not sure what the value of "testing" a known-incorrect
grammar might be. It wouldn't necessarily act the same after being
fixed.

Well surely the c code the parser invokes will behave the same. A lot
of c hackers are not bison grammar hackers. Even many of us former
bison grammar hackers are way rusty. There have been a number of times
when someone has posted an otherwise working patch with a grammar
conflict you fixed

Bruce surely nobody would object if you posted a path to add a
comment. People would of course quibble with the wording but that's
just par for the course.

Perhaps something like "postgres jas a policy of maintaining zero
parser conflicts. If you disable this for testing make sure you re-
enable it and eliminate any conflicts. Or post to -hackers asking for
advice"

I'm not sure where to put a comment pointing them to the %expected
line though. What does the error look like if they violate it?