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

Problem with bytes &size=self.offset() #1842

Closed
sethhall opened this issue Aug 27, 2024 · 3 comments · Fixed by #1877
Closed

Problem with bytes &size=self.offset() #1842

sethhall opened this issue Aug 27, 2024 · 3 comments · Fixed by #1877
Assignees
Labels
Bug Something isn't working

Comments

@sethhall
Copy link
Member

I'm not sure if this is related to some expression parsing issue in &size attributes on bytes fields or something else, but I'm seeing a weird behavior...

This works as you might expect (parse 3 bytes then calculate the crc32 for those bytes)...

module Test;
import spicy;

public type X = unit {
    a: uint8;
    b: uint8;
    c: uint8;
    : bytes &size=3 &parse-at=self.input() {
        self.crc32 = spicy::crc32_add(spicy::crc32_init(), $$);
    }
    var crc32: uint64;
};

But this doesn't work (even though self.offset() returns 3)...

module Test;
import spicy;

public type X = unit {
    a: uint8;
    b: uint8;
    c: uint8;
    : bytes &size=self.offset() &parse-at=self.input() {
        self.crc32 = spicy::crc32_add(spicy::crc32_init(), $$);
    }
    var crc32: uint64;
};

It seems that self.offset() used in the &size field ends up returning zero?

@sethhall sethhall changed the title Expressions in &size? Problem with bytes &size=self.offset() Aug 27, 2024
@rsmmr rsmmr added the Bug Something isn't working label Aug 28, 2024
@rsmmr
Copy link
Member

rsmmr commented Aug 28, 2024

Oh ... Never thought about this but the way offset() is currently implemented, means its works only inside hooks, not from attributes. We should fix that.

@rsmmr
Copy link
Member

rsmmr commented Sep 27, 2024

It's a different issue actually: it's the &parse-at that gets applied before offset() is evaluated, meaning offset is now zero indeed. That's not intuitive, fixing.

@rsmmr rsmmr self-assigned this Sep 27, 2024
rsmmr added a commit that referenced this issue Sep 27, 2024
With `&parse-at/from` we were updating the internal state on our
current position immediately, meaning they were visible already when
evaluating other attributes on the same field afterwards, which is
unexpected.

Closes #1842.
@sethhall
Copy link
Member Author

Ok, that bug is pretty unintuitive. :)

rsmmr added a commit that referenced this issue Sep 30, 2024
With `&parse-at/from` we were updating the internal state on our
current position immediately, meaning they were visible already when
evaluating other attributes on the same field afterwards, which is
unexpected.

Closes #1842.
@rsmmr rsmmr closed this as completed in 2eeffa6 Sep 30, 2024
rsmmr added a commit that referenced this issue Sep 30, 2024
* origin/topic/robin/gh-1842-offset:
  Fix when input redirection becomes visible.
bbannier pushed a commit that referenced this issue Oct 1, 2024
With `&parse-at/from` we were updating the internal state on our
current position immediately, meaning they were visible already when
evaluating other attributes on the same field afterwards, which is
unexpected.

Closes #1842.

(cherry picked from commit 2eeffa6)
bbannier pushed a commit that referenced this issue Oct 1, 2024
With `&parse-at/from` we were updating the internal state on our
current position immediately, meaning they were visible already when
evaluating other attributes on the same field afterwards, which is
unexpected.

Closes #1842.

(cherry picked from commit 2eeffa6)
bbannier pushed a commit that referenced this issue Oct 2, 2024
With `&parse-at/from` we were updating the internal state on our
current position immediately, meaning they were visible already when
evaluating other attributes on the same field afterwards, which is
unexpected.

Closes #1842.

(cherry picked from commit 2eeffa6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants