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

Replace assert statements in Marshall.hpp with run-time error handling #16

Closed
richey-v opened this issue Mar 9, 2021 · 2 comments
Closed
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@richey-v
Copy link
Contributor

richey-v commented Mar 9, 2021

The assert statements were discovered due to a warning, reported in #13. The warnings were addressed in #15.

The apparent purpose of the asserts is to ensure that a NewByteArray can be created that is large enough to handle the data to be marshaled. There is no run-time error handling other than the assert statement, so if those are optimized away, there is no safety at all.

@a4z a4z added enhancement New feature or request help wanted Extra attention is needed labels Mar 9, 2021
@a4z
Copy link
Contributor

a4z commented Mar 9, 2021

I agree that there is no safety , except a check in a debug build (with an assert failure).

But I have difficult to see what else to do.
Exceptions, just crash .... ? This might result in the same , because, how could you handle such an exception.
And then there is of course the question of the likeliness of such an event, and if we are willy to pay the price of additional code in the release build for this scenario?

It would be good to find answers to those questions to come up with a strategy for a better solution, if there is any

@a4z
Copy link
Contributor

a4z commented Jun 24, 2021

After skimming again though our assert usages, I am closing that issue.

Asserts have a defined use case in C++, that is, check a condition in debug build and don't check (remove that code) in release builds.
There are many asserts, and they are used in the spirit of that use case.

If there is any special assert that should be implemented as a runtime exception also in release builds, there should be specific ticket for it, so it can be discussed. And if there is an exception implementation, tests need be added to trigger that problem and verify exceptions are thrown accordingly.

A general ticket like this, that wants to remove all asserts, can not be implemented, since asserts have a real use case.

@a4z a4z closed this as completed Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants