Nothing Special   »   [go: up one dir, main page]

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ CPP ] Throw runtime exception on parsing error #288

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

miketsukerman
Copy link
@miketsukerman miketsukerman commented Jan 26, 2020

When BNFC generated code embedded into another project it's more convenient to get parsing errors as C++ exceptions rather than just via standard output.

I've added parse_error class to the Absyn.H

class parse_error : public std::runtime_error {
  public:
  parse_error(int line, std::string str) : std::runtime_error(str) , m_line(line) {}
  int getLine() { return m_line; }
  private:
  int m_line;
  };

@andreasabel andreasabel changed the title [ CPP ] Thow runtime exception on parsing error [ CPP ] Throw runtime exception on parsing error Jan 27, 2020
Copy link
Member
@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on the CPP backend!

Here is a few comments on how to improve this PR:

  • Since the exception concerns only parsing, its definition should not be in the file for the abstract syntax but in the one for the parser.
  • Make sure the generated code is properly indented.
  • I wonder whether the generated test program also needs to be updated, to catch the exception and print the error message that was used to be printed directly.
  • Finally, the desired behavior you are implemented should be secured by a test case. See the testing directory for examples of tests for functionality.

@miketsukerman miketsukerman force-pushed the throw-runtime-exception branch 2 times, most recently from 5d170fb to 3c247de Compare March 9, 2020 21:08
@miketsukerman
Copy link
Author

Hi @andreasabel, I moved the definition of parser_exception to the separate header. Would it be okay?

@miketsukerman
Copy link
Author

Regarding the tests. To be honest, not that much proficient in Haskell. I would be very pleased if you could help me with tests or at least explain to me how to extend them ;)

@andreasabel
Copy link
Member

Thanks for the update, @miketsukerman ! At the moment I am a bit snowed under, but I will look at the PR when things are back to normal.

@andreasabel
Copy link
Member
andreasabel commented Mar 20, 2020

Hi Mike,
with your patch the standard testsuite fails for C++ (with namespace), e.g.:

[TEST] Parameterized tests:C++ (with namespace):Examples:LBNF
bnfc -mMyMakefile --cpp '-p foobar' LBNF.cf
make -f MyMakefile
error running: make -f MyMakefile
exit status: 2
stderr: LBNF.y:26:18: error: no member named 'parse_error' in namespace 'foobar'
  throw  foobar::parse_error( foobaryy_mylinenumber,str);
         ~~~~~~~~^
1 error generated.

One way to run the test is simply to type make in the bnfc root folder.

The generated ParseError.H typically looks like this:

 namespace bnfc
 {
 class parse_error : public std::runtime_error
 {
 public:
     parse_error(int line, std::string str)
         : std::runtime_error(str)
         , m_line(line) {}
...

using its own namespace bnfc.

But the call to parse_error in Parser.C looks like this:

throw foo::parse_error(fooyy_mylinenumber,str);

expecting parse_error in the namespace as declared by the user.

If parse_error would be placed into Parser.H I think this would work automatically, because Parser.H uses the namespace as given by the -p flag to BNFC.

However, you might have strong reasons to prefer a separate .H file. (Then, of course, you would also have to use the namespace as given by the user.)
Could you explain your rationale?

@andreasabel andreasabel added C++ enhancement parser Issues concerning parser generating labels Mar 20, 2020
@miketsukerman
Copy link
Author

Hi Mike,
with your patch the standard testsuite fails for C++ (with namespace), e.g.:

[TEST] Parameterized tests:C++ (with namespace):Examples:LBNF
bnfc -mMyMakefile --cpp '-p foobar' LBNF.cf
make -f MyMakefile
error running: make -f MyMakefile
exit status: 2
stderr: LBNF.y:26:18: error: no member named 'parse_error' in namespace 'foobar'
  throw  foobar::parse_error( foobaryy_mylinenumber,str);
         ~~~~~~~~^
1 error generated.

One way to run the test is simply to type make in the bnfc root folder.

The generated ParseError.H typically looks like this:

 namespace bnfc
 {
 class parse_error : public std::runtime_error
 {
 public:
     parse_error(int line, std::string str)
         : std::runtime_error(str)
         , m_line(line) {}
...

using its own namespace bnfc.

But the call to parse_error in Parser.C looks like this:

throw foo::parse_error(fooyy_mylinenumber,str);

expecting parse_error in the namespace as declared by the user.

If parse_error would be placed into Parser.H I think this would work automatically, because Parser.H uses the namespace as given by the -p flag to BNFC.

However, you might have strong reasons to prefer a separate .H file. (Then, of course, you would also have to use the namespace as given by the user.)
Could you explain your rationale?

Very simple - just a mistake. Fixed

@andreasabel
Copy link
Member

Thanks. Er, I meant the rationale of having a separate .H file.

@miketsukerman
Copy link
Author
miketsukerman commented Mar 20, 2020

Thanks. Er, I meant the rationale of having a separate .H file.

It was the simplest way to include it in the <bison>.y

#include "ParserError.H"
#include "Absyn.H"

#define YYMAXDEPTH 10000000

typedef struct yy_buffer_state *YY_BUFFER_STATE;
int yyparse(void);
int yylex(void);
YY_BUFFER_STATE bnfcyy_scan_string(const char *str);
void bnfcyy_delete_buffer(YY_BUFFER_STATE buf);
int bnfcyy_mylinenumber;
void bnfcinitialize_lexer(FILE * inp);
int bnfcyywrap(void)
{
  return 1;
}
void bnfcyyerror(const char *str)
{
  extern char *bnfcyytext;
  throw bnfc::parse_error(bnfcyy_mylinenumber,str);
}

I can't include Parser.H here. And I can't use forward declaration here as well unfortunately.

@andreasabel
Copy link
Member

Ok, I see, thanks for the explanation!

I ran the testsuite on your PR and now there is only a small problem left:

* Failures:
  * Parameterized tests:C++:distclean removes all generated files
  * Parameterized tests:C++ (with namespace):distclean removes all generated files

This means that the cleaning goals of the generated Makefile for the CPP-STL backend (the one you changed) need to be extended by the new file ParserError.H.

I can fix this myself. Thanks for your contribution!

Copy link
Member
@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the review I should have done before merging, haha.

The patch introduced some regressions, unfortunately.

source/src/BNFC/Backend/CPP/STL.hs Show resolved Hide resolved
source/src/BNFC/Backend/CPP/STL.hs Show resolved Hide resolved
andreasabel added a commit that referenced this pull request Oct 29, 2020
We generated C++98 code so far, PR #288 broke this, revealed by the
strict GNU C++ compiler (clang does not notice this).

Using NULL instead of nullptr should bring us back to "ansi" C++98.
andreasabel added a commit that referenced this pull request Oct 29, 2020
- unused variable yytext in parse error function
- parse errors should be reported on stderr as before
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ enhancement parser Issues concerning parser generating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants