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

Provide meaningful unit __begin value when parsing starts. #1650

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

bbannier
Copy link
Member

We previously would not provide __begin when starting the initial parse. This meant that e.g., offset() was not usable if nothing ever got parsed.

With this patch we provide a meaningful value now.

Closes #1648.

@bbannier bbannier self-assigned this Jan 15, 2024
@bbannier bbannier marked this pull request as ready for review January 15, 2024 11:08
@bbannier bbannier requested a review from rsmmr January 15, 2024 11:08
Copy link
Member

@rsmmr rsmmr left a comment

Choose a reason for hiding this comment

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

I'm trying to recall if there was a reason for the current behavior. This is actually documented even, see https://docs.zeek.org/projects/spicy/en/latest/programming/language/types.html#method-unit::offset and other random access functions. So we will need to update the documentation at least.

@bbannier
Copy link
Member Author

I'm trying to recall if there was a reason for the current behavior. This is actually documented even, see https://docs.zeek.org/projects/spicy/en/latest/programming/language/types.html#method-unit::offset and other random access functions. So we will need to update the documentation at least.

Good question. This docstring seems to have existed before we introduced the new explicit tracking of the offset value, but the parse methods still allow for parsing without specifying the a begin so it seems possible that it could still unset. I'll investigate.

@bbannier bbannier requested a review from rsmmr January 16, 2024 10:39
rsmmr
rsmmr previously approved these changes Jan 16, 2024
spicy/toolchain/src/compiler/codegen/parser-builder.cc Outdated Show resolved Hide resolved
@bbannier bbannier requested a review from rsmmr January 17, 2024 11:58
rsmmr
rsmmr previously approved these changes Jan 17, 2024
We previously would not provide `__begin` when starting the initial
parse. This meant that e.g., `offset()` was not usable if nothing ever
got parsed.

With this patch we provide a meaningful value now.

Closes #1648.
The originally documented caveats do not apply anymore, so remove them.
@bbannier bbannier force-pushed the topic/bbannier/issue-1648 branch from 3b8d5f4 to ddd7535 Compare January 17, 2024 16:28
@bbannier bbannier merged commit 165fb54 into main Jan 18, 2024
19 checks passed
@bbannier bbannier deleted the topic/bbannier/issue-1648 branch January 18, 2024 07:49
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.

Obscure UnsetOptional error on assert from %done hook
2 participants