Skip to content

Replace impossible error conditions with debug assertions #169

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

apasel422
Copy link
Collaborator

These conditions are only possible when the individual bare-item parsing
methods are called directly, but that is only possible in tests.

@apasel422 apasel422 marked this pull request as ready for review March 31, 2025 12:43
@apasel422 apasel422 requested a review from valenting April 1, 2025 12:28
@apasel422 apasel422 requested a review from undef1nd April 15, 2025 12:44
Copy link
Collaborator

@valenting valenting left a comment

Choose a reason for hiding this comment

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

As I mentioned in a previous PR, I'm not sure we ought to replace these checks with debug asserts. These don't run in production, and leave space for obscure bugs to creep in.

@apasel422
Copy link
Collaborator Author

As I mentioned in a previous PR, I'm not sure we ought to replace these checks with debug asserts. These don't run in production, and leave space for obscure bugs to creep in.

If these methods were inlined into their lone caller, these error conditions would clearly be impossible. We could do that instead, guaranteeing that this code cannot be reached without the proper precondition. IMO these non-public methods only exist for readability, not because we expect them to gain any other callers in the future.

@apasel422 apasel422 force-pushed the assert branch 2 times, most recently from 627d539 to 5da5108 Compare May 19, 2025 12:53
apasel422 added 4 commits June 5, 2025 08:08
These conditions are only possible when the individual bare-item parsing
methods are called directly, but that is only possible in tests.
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