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

Stop using runtime C++ exceptions #145

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maelhos
Copy link

@maelhos maelhos commented Jan 2, 2024

Replace throw by std::exit

As of LLVM 18, llvm-config --cxxflags contains -fno-exceptions, general standards are also moving out runtime exceptions in C++ as much as possible c.f. stackoverflow discussion.

I can't run test since the dev is broken because of transition from babel 6 to 7 (I don't know enough about babel to update everything to babel 7) and there is no version nor instruction specifying the required npm and node.

As of LLVM 18, llvm-config contains -fno-exceptions, general standard are also moving off runtime exceptions in C++ https://stackoverflow.com/questions/4506369/when-and-how-should-i-use-exception-handling .
@@ -204,7 +204,7 @@ class Tokenizer {
<< ":" << column << "\n\n";

std::cerr << errMsg.str();
throw new std::runtime_error(errMsg.str().c_str());
std::exit(1);
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be EXIT_FAILURE as well?

@DmitrySoshnikov
Copy link
Owner

@maelhos, thanks for the contribution! Hopefully this is not a breaking change anywhere (except the cases which may catch for the specific exceptions, but that should be fine). Can you confirm the EXIT_FAILURE in the second case?

@maelhos
Copy link
Author

maelhos commented Jan 3, 2024

For that you would need stdlib.h, it doesn't seem to be included in this part of the template though it probably is in the final header, I used 1 to play it safe but if you have the ability to test if EXIT_FAILURE works that would be great.

@DmitrySoshnikov
Copy link
Owner

You can add it here: https://github.com/DmitrySoshnikov/syntax/blob/master/src/plugins/cpp/templates/lr.template.h#L23

Where you able to test the generated code? I'll try looking at #141 later if you're blocked on testing, unless you reach that issue earlier.

@maelhos
Copy link
Author

maelhos commented Jan 4, 2024

I dont't think it is possible to test generation without solving #141 since it happens at compile time though the package syntax-cli available through npm still works fine.

@DmitrySoshnikov
Copy link
Owner

#141 should be fixed 0ec6737. Can you verify?

@maelhos
Copy link
Author

maelhos commented Jan 6, 2024

#141 should be fixed 0ec6737. Can you verify?

It's fixed, everything works for me, instead of just exiting we could also make a custom Error Wrapper which could be overwritten by the user (kinda yyerror in bison) would you want that ?

@DmitrySoshnikov
Copy link
Owner

Yes, good idea - we can probably change it in the follow up PR.

For this one - can you update please to add stdlib.h in https://github.com/DmitrySoshnikov/syntax/blob/master/src/plugins/cpp/templates/lr.template.h#L23 and also change 1 to EXIT_FAILURE?

@DmitrySoshnikov
Copy link
Owner

Actually, a program should be able to handle parse error at runtime. With the full exit this now changes semantics, right? - you can't catch the parse errors anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants