Loading ...
Sorry, an error occurred while loading the content.

Re: Error reporting - Complete patch for 2.7.1

Expand Messages
  • Michael Schmitt
    Hello, unfortunately, I have not covered the special case that a fileName might be passed as null to the constructor of RecognitionException in the Java
    Message 1 of 15 , Oct 1, 2000
    • 0 Attachment
      Hello,

      unfortunately, I have not covered the special case that a fileName might be
      passed as "null" to the constructor of "RecognitionException" in the Java code
      :-(

      Enclosed please find an updated patch with a revised
      "RecognitionException.java" and a cosmetic change in
      "RecognitionException.cpp".

      You can install the updated patch as described in my previous email (see
      below). For the sake of completeness and convenience I have created a new tar
      file with a complete set of modified files.

      Any comments are welcome.

      Michael

      > You can install the patches very easily:
      >
      > 1. Install ANTLR 2.7.1 release candidate
      > 2. Unpack patch.tar.gz and overwrite the files of 2.7.1 by the ones in
      > patch.tar
      > 3. Invoke the ./build script (ignore errors)
      > 4. Recompile all "*.g" files in ./antlr and its subdirectories:
      > ./antlr/actions/cpp/action.g
      > ./antlr/actions/java/action.g
      > ./antlr/actions/sather/action.g
      > ./antlr/antlr.g
      > ./antlr/preprocessor/preproc.g
      > ./antlr/tokdef.g
      > 5. Invoke the ./build script again (there should be not errors this time)

      --
      ======================================================================
      Michael Schmitt phone: +49 451 500 3725
      Institute for Telematics secretary: +49 451 500 3721
      Medical University of Luebeck fax: +49 451 500 3722
      Ratzeburger Allee 160 eMail: schmitt@...-luebeck.de
      D-23538 Luebeck, Germany WWW: http://www.itm.mu-luebeck.de
      ======================================================================
    • Ric Klaren
      schmitt@itm.mu-luebeck.de on Sat, Sep 30, 2000 at 06:28:31PM +0200 Hi, On Sat, Sep 30, 2000 at 06:28:31PM +0200, Michael Schmitt wrote: I finally managed to
      Message 2 of 15 , Oct 2, 2000
      • 0 Attachment
        schmitt@...-luebeck.de on Sat, Sep 30, 2000 at 06:28:31PM +0200

        Hi,

        On Sat, Sep 30, 2000 at 06:28:31PM +0200, Michael Schmitt wrote:
        I finally managed to complete the patch for improved error reporting.
        Parsers generated by ANTLR (based on Java and C++) will report errors with
        filename, line and column information now. The output format is chosen in
        a way that (x)emacs directly jumps to the erroneous file (at the right
        line/column) if you click on the error message.

        Great patch :) Allows me too strike exception class rework from the Todo
        list :)

        You can install the patches very easily:

        When it comes to patches I (personally) prefer diffs it is a lot easier to
        see what really changes with those. Preferably diff -uwbBpPr oldir/ newdir/.

        Which leads up to my question:

        @@ -1284,21 +1280,9 @@ public class JavaCodeGenerator extends C
        // It is a token or literal reference. Generate the
        // correct variable type for this grammar
        println(labeledElementType + " " + a.getLabel() + " = "$
        - // In addition, generate *_AST variables if
        - // building ASTs
        + // In addition, generate *_AST variables if building AS$
        if (grammar.buildAST) {
        - if (a instanceof GrammarAtom &&
        - ((GrammarAtom)a).getASTNodeType()!=null ) {
        - GrammarAtom ga = (GrammarAtom)a;
        - println(ga.getASTNodeType()+" " +
        - a.getLabel() +
        - "_AST = null;");
        - }
        - else {
        - println(labeledElementASTType+" " +
        - a.getLabel() +
        - "_AST = null;");
        - }
        + println(labeledElementASTType+" " + a.getLabel() + $
        }
        }
        }

        Is this a change you made to the JavaCodegenerator, Michael, or? ('-'
        prefixed lines are the antlr code disappearing the '+' prefixed lines are
        the ones introduced by you) Just checking....

        Rest of the patch looks fine!

        > I would be very grateful if some guys could test the patch. The changes
        > are pretty simple and should not cause any new problem.

        Remember how often I said that =) (Not that this patch caused any trouble
        of course (yet ;) ) just saying that I've grown a little suspicious of
        small patches that really don't change anything even if I made them myself)

        BTW Terence. How is development going to continue? In the main branch?
        Shall I include the C++ changes of this patch? Or you do all?

        BTW2 Unicode support is not working in C++ mode this could be stated in the
        2.7.1 docs. If I recall right. This will need some changes to the support
        classes. The lexer and probably also Parser/TreeParser need to be
        templatized with the character type used (wchar_t for unicode) to make this
        work.

        BTW3 The website does not mention 2.7.1, yet...

        Ric
        --
        -----+++++*****************************************************+++++++++-------
        -- Ric Klaren - klaren@... ------------------------------------------
        -----+++++*****************************************************+++++++++-------
        'And this 'rebooting' business? Give it a good kicking, do you?' 'Oh, no,
        of course, we ... that is ... well, yes, in fact,' said Ponder. 'Adrian
        goes round the back and ... er ... prods it with his foot. But in a
        technical way,' he added. --- From: Hogfather by Terry Pratchett.
        -------------------------------------------------------------------------------
      • Michael Schmitt
        ... Thank you. Enclosed please find updated patch files in which I have included Terence s last-minute changes. You should be able to simply overwrite your
        Message 3 of 15 , Oct 2, 2000
        • 0 Attachment
          Ric Klaren wrote:

          > Great patch :) Allows me too strike exception class rework from the Todo
          > list :)

          Thank you. Enclosed please find updated patch files in which I have included
          Terence's last-minute changes. You should be able to simply overwrite your
          files at no risk (I spent many hours in the most pedantic way to make sure that
          I did not introduce new bugs - and the patch is not very spectacular
          technically).

          > When it comes to patches I (personally) prefer diffs it is a lot easier to
          > see what really changes with those. Preferably diff -uwbBpPr oldir/ newdir/.

          The real changes are marginal. A "diff" would uncover a lot of modifications
          that were only made for aethestic reasons or in order to make the C++ and Java
          code look as similar as possible.

          > Is this a change you made to the JavaCodegenerator, Michael, or?
          > ...

          No. This is one of Terence's patches made after the candidate release.

          > Remember how often I said that =) (Not that this patch caused any trouble
          > of course (yet ;) ) just saying that I've grown a little suspicious of
          > small patches that really don't change anything even if I made them myself)

          That is probably the reason why the patch is not in 2.7.1. Of course, I am a
          little bit sad about that. But if I remember correctly, Terence offered to
          release 2.7.2a1, didn't he!?

          > Shall I include the C++ changes of this patch? Or you do all?

          I hope the patch will be put into the source repository as soon as possible
          because you can image I would not like to inspect it another times. Please
          note that there are also changes in the Java classes (this was said to be a
          prerequisite for code merging)

          Michael

          --
          ======================================================================
          Michael Schmitt phone: +49 451 500 3725
          Institute for Telematics secretary: +49 451 500 3721
          Medical University of Luebeck fax: +49 451 500 3722
          Ratzeburger Allee 160 eMail: schmitt@...-luebeck.de
          D-23538 Luebeck, Germany WWW: http://www.itm.mu-luebeck.de
          ======================================================================
        • Michael Schmitt
          ... Well, for the moment, the output is fixed. But it should be fairly simple to make it customizable. You only have to replace the calls to
          Message 4 of 15 , Oct 2, 2000
          • 0 Attachment
            Robert Colquhoun wrote:

            > Sorry for the dumb question - but i was looking at your patch and you seem
            > to have removed customizable error reporting from the exceptions....What
            > happens if i don't want line + column number reporting in the exception output?

            Well, for the moment, the output is fixed. But it should be fairly simple to
            make it customizable.

            You only have to replace the calls to "getFileLineColumnString()" in
            'RecognitionException.java/cpp' by calls to another method. Then at the same
            time you can delete the method above which becomes superfluent.

            One thing I am not very happy about it the fact that there are too many
            different functions for reporting errors and warnings, as you can see when
            running ANTLR itself.

            I would appreciate it very much if you clean up the existing code (both Java
            and C++) in a way that there is only one method (or maybe two) for reporting
            errors and warnings. I would have done it by myself but my ressources are
            limited (ANTLR is just a meta problem for me).

            Nevertheless, I think my patch is a first small step in the right direction and
            I would like to see it taken into the source code repository because other
            motivated ANTLR developers start turning everyhting inside out :-)

            Kind regards,

            Michael

            --
            ======================================================================
            Michael Schmitt phone: +49 451 500 3725
            Institute for Telematics secretary: +49 451 500 3721
            Medical University of Luebeck fax: +49 451 500 3722
            Ratzeburger Allee 160 eMail: schmitt@...-luebeck.de
            D-23538 Luebeck, Germany WWW: http://www.itm.mu-luebeck.de
            ======================================================================
          • Ross Bencina
            ... At which point I jump in to reiterate my suggestion for a MessageLog strategy abstract class. Adding a class to support error/warning logging would allow
            Message 5 of 15 , Oct 2, 2000
            • 0 Attachment
              Michael Schmitt mailto:schmitt@...-luebeck.de> wrote:
              > One thing I am not very happy about it the fact that there are too many
              > different functions for reporting errors and warnings, as you can see when
              > running ANTLR itself.

              At which point I jump in to reiterate my suggestion for a MessageLog
              strategy abstract class.

              Adding a class to support error/warning logging would allow users to
              substitute their own message handling on a per-parser basis using something
              like:

              MyMessageLog ml;

              myParser->setMessageLog( &ml ); // override default message handling

              The standard implementation would output to the standard streams as is
              currently the case, but custom implementations could be written by users.
              The utility of this may be lost on command-line unix users, but when a
              parser is embedded in a GUI app it would be nice to have a clean OO
              interface to message handling: for example the custom MessageLog could store
              all received messages in a vector<> and then update a list view when the
              parser has finished (bad example, but you get the idea...)

              See my message 15th September "proposal: MessageLog Strategy refactoring"
              for further rationale and details.

              Best wishes,

              Ross.
            • Robert Colquhoun
              Hi Michael, Sorry for the dumb question - but i was looking at your patch and you seem to have removed customizable error reporting from the exceptions....What
              Message 6 of 15 , Oct 2, 2000
              • 0 Attachment
                Hi Michael,

                Sorry for the dumb question - but i was looking at your patch and you seem
                to have removed customizable error reporting from the exceptions....What
                happens if i don't want line + column number reporting in the exception output?

                PS I had trouble reading the changes so i might of missed this(conditioned
                to look at diff files...)

                - Robert
              • Ric Klaren
                ... ACK! ... Hadn t forgotten about that one =) Ric -- ... -- Ric Klaren - klaren@cs.utwente.nl ------------------------------------------ ... And this
                Message 7 of 15 , Oct 3, 2000
                • 0 Attachment
                  On Tue, Oct 03, 2000 at 06:25:35AM +1000, Ross Bencina wrote:
                  > Michael Schmitt mailto:schmitt@...-luebeck.de> wrote:
                  > > One thing I am not very happy about it the fact that there are too many
                  > > different functions for reporting errors and warnings, as you can see when
                  > > running ANTLR itself.
                  >
                  > Adding a class to support error/warning logging would allow users to
                  > substitute their own message handling on a per-parser basis using something
                  > like:

                  ACK!

                  > See my message 15th September "proposal: MessageLog Strategy refactoring"
                  > for further rationale and details.

                  Hadn't forgotten about that one =)

                  Ric
                  --
                  -----+++++*****************************************************+++++++++-------
                  -- Ric Klaren - klaren@... ------------------------------------------
                  -----+++++*****************************************************+++++++++-------
                  'And this 'rebooting' business? Give it a good kicking, do you?' 'Oh, no,
                  of course, we ... that is ... well, yes, in fact,' said Ponder. 'Adrian
                  goes round the back and ... er ... prods it with his foot. But in a
                  technical way,' he added. --- From: Hogfather by Terry Pratchett.
                  -------------------------------------------------------------------------------
                • Ric Klaren
                  ... Seems I forgot to sync one time. :) ... Okie... ... Somewhere near the big unicode announcement (a except for C++ mode atm would be sufficient I think) :)
                  Message 8 of 15 , Oct 3, 2000
                  • 0 Attachment
                    On Tue, Oct 03, 2000 at 10:20:00AM -0700, Terence Parr wrote:
                    > I made this in response to label types for hetero trees from Jerry
                    > James (last night) ;)

                    Seems I forgot to sync one time. :)

                    > Tweaks go in the mainline, but in general dev should happen in your
                    > dev branch. mainline is essentially stuff that compiles, works, and
                    > is to be included in next release. Be sure to do a

                    Okie...

                    > > BTW2 Unicode support is not working in C++ mode this could be stated in the
                    > > 2.7.1 docs. If I recall right. This will need some changes to the support
                    > > classes. The lexer and probably also Parser/TreeParser need to be
                    > > templatized with the character type used (wchar_t for unicode) to make this
                    > > work.
                    >
                    > Shoot...where should we stick it?

                    Somewhere near the big unicode announcement (a except for C++ mode atm
                    would be sufficient I think) :)

                    Cheers,

                    Ric
                    --
                    -----+++++*****************************************************+++++++++-------
                    -- Ric Klaren - klaren@... ------------------------------------------
                    -----+++++*****************************************************+++++++++-------
                    'And this 'rebooting' business? Give it a good kicking, do you?' 'Oh, no,
                    of course, we ... that is ... well, yes, in fact,' said Ponder. 'Adrian
                    goes round the back and ... er ... prods it with his foot. But in a
                    technical way,' he added. --- From: Hogfather by Terry Pratchett.
                    -------------------------------------------------------------------------------
                  • Ross Bencina
                    ... refactoring ... Ok, I m off on a road trip until next monday, I ll implement it when I get back (unless someone else beats me to it)... presumably 2.7.2a1
                    Message 9 of 15 , Oct 3, 2000
                    • 0 Attachment
                      Ric Klaren <klaren@...> wrote:
                      > > See my message 15th September "proposal: MessageLog Strategy
                      refactoring"
                      > > for further rationale and details.
                      >
                      > Hadn't forgotten about that one =)

                      Ok, I'm off on a road trip until next monday, I'll implement it when I get
                      back (unless someone else beats me to it)... presumably 2.7.2a1 with the
                      other error patches included will be available by then...

                      Best wishes,

                      Ross.
                    • Terence Parr
                      ... I made this in response to label types for hetero trees from Jerry James (last night) ;) ... Tweaks go in the mainline, but in general dev should happen in
                      Message 10 of 15 , Oct 3, 2000
                      • 0 Attachment
                        Monday, October 02, 2000, Ric Klaren hath spoken:
                        > schmitt@...-luebeck.de on Sat, Sep 30, 2000 at 06:28:31PM +0200

                        > Hi,

                        > On Sat, Sep 30, 2000 at 06:28:31PM +0200, Michael Schmitt wrote:
                        > I finally managed to complete the patch for improved error reporting.
                        > Parsers generated by ANTLR (based on Java and C++) will report errors with
                        > filename, line and column information now. The output format is chosen in
                        > a way that (x)emacs directly jumps to the erroneous file (at the right
                        > line/column) if you click on the error message.

                        > Great patch :) Allows me too strike exception class rework from the Todo
                        > list :)

                        > You can install the patches very easily:

                        > When it comes to patches I (personally) prefer diffs it is a lot easier to
                        > see what really changes with those. Preferably diff -uwbBpPr oldir/ newdir/.

                        > Which leads up to my question:

                        > @@ -1284,21 +1280,9 @@ public class JavaCodeGenerator extends C
                        > // It is a token or literal reference. Generate the
                        > // correct variable type for this grammar
                        > println(labeledElementType + " " + a.getLabel() + " = "$
                        > - // In addition, generate *_AST variables if
                        > - // building ASTs
                        > + // In addition, generate *_AST variables if building AS$
                        > if (grammar.buildAST) {
                        > - if (a instanceof GrammarAtom &&
                        > - ((GrammarAtom)a).getASTNodeType()!=null ) {
                        > - GrammarAtom ga = (GrammarAtom)a;
                        > - println(ga.getASTNodeType()+" " +
                        > - a.getLabel() +
                        > - "_AST = null;");
                        > - }
                        > - else {
                        > - println(labeledElementASTType+" " +
                        > - a.getLabel() +
                        > - "_AST = null;");
                        > - }
                        > + println(labeledElementASTType+" " + a.getLabel() + $
                        > }
                        > }
                        > }

                        > Is this a change you made to the JavaCodegenerator, Michael, or? ('-'

                        I made this in response to label types for hetero trees from Jerry
                        James (last night) ;)

                        > BTW Terence. How is development going to continue? In the main branch?
                        > Shall I include the C++ changes of this patch? Or you do all?

                        Tweaks go in the mainline, but in general dev should happen in your
                        dev branch. mainline is essentially stuff that compiles, works, and
                        is to be included in next release. Be sure to do a

                        p4 sync
                        p4 integrate -b klaren.dev
                        p4 resolve -at # accept all changes from main
                        p4 submit

                        from your branch to get a clean copy of main before continuing on.

                        I would like to examine the exception hierarchy personally before we
                        decide on a path there. I'm sure Michael has done most of the
                        thinking for us though! ;):D

                        > BTW2 Unicode support is not working in C++ mode this could be stated in the
                        > 2.7.1 docs. If I recall right. This will need some changes to the support
                        > classes. The lexer and probably also Parser/TreeParser need to be
                        > templatized with the character type used (wchar_t for unicode) to make this
                        > work.

                        Shoot...where should we stick it?

                        > BTW3 The website does not mention 2.7.1, yet...

                        Ooops...yeah I'll get to that soon.

                        Thanks,
                        Ter
                        --
                        Chief Scientist & Co-founder, http://www.jguru.com
                        Co-founder, http://www.NoWebPatents.org -- Stop Patent Stupidity
                        parrt@...
                      • Terence Parr
                        ... First rule of releasing software: release it ;) Doing last minute patches is very dangerous and as you saw the one last patch stretched 2.7.1 for a few
                        Message 11 of 15 , Oct 3, 2000
                        • 0 Attachment
                          Monday, October 02, 2000, Michael Schmitt hath spoken:
                          >> Remember how often I said that =) (Not that this patch caused any trouble
                          >> of course (yet ;) ) just saying that I've grown a little suspicious of
                          >> small patches that really don't change anything even if I made them myself)

                          > That is probably the reason why the patch is not in 2.7.1. Of course, I am a
                          > little bit sad about that.

                          First rule of releasing software: release it ;) Doing last minute
                          patches is very dangerous and as you saw the "one last patch"
                          stretched 2.7.1 for a few months! ;)

                          > But if I remember correctly, Terence offered to
                          > release 2.7.2a1, didn't he!?

                          You bet! I need to actually turn on my brain though for the exception
                          hiearchy...

                          BTW, I actually prefer full files not changes. I have tkdiff, which
                          does a great job of telling me the changes whereas patch just says
                          "uh, i can't apply the patch" when my files change. Inevitably my
                          files will change before I can get to the patch ;)

                          >> Shall I include the C++ changes of this patch? Or you do all?

                          > I hope the patch will be put into the source repository as soon as possible
                          > because you can image I would not like to inspect it another times. Please
                          > note that there are also changes in the Java classes (this was said to be a
                          > prerequisite for code merging)

                          Ric, how about you freshen your dev branch and apply Michael's changes
                          to check it out. Then I can take a look at the Java part etc...

                          Ter
                          --
                          Chief Scientist & Co-founder, http://www.jguru.com
                          Co-founder, http://www.NoWebPatents.org -- Stop Patent Stupidity
                          parrt@...
                        Your message has been successfully submitted and would be delivered to recipients shortly.