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

How to make type of a field depend on field in another variable #106

Closed
KOLANICH opened this issue Feb 4, 2017 · 8 comments
Closed

How to make type of a field depend on field in another variable #106

KOLANICH opened this issue Feb 4, 2017 · 8 comments
Assignees
Labels

Comments

@KOLANICH
Copy link

KOLANICH commented Feb 4, 2017

seq:
...
  - id: f_version
    doc: "File format version"
    type: u4
...
types:
  offset_ver:
    seq:
      - id: offset
        type: u8
        if:  'root.f_version > 1000000'
      - id: offset
        type: u4
        if:  'root.f_version <= 1000000'

causes
KS compilation error: java.lang.RuntimeException: Unable to access root in List(root, offset_ver) context

How to fix?

@GreyCat
Copy link
Member

GreyCat commented Feb 4, 2017

First of all, you've probably meant _root, not root.

Then you'll encounter that you can't have duplicate fields with the same ID. There are two solutions available right now:

  1. Read it as two distinct fields, then unite them using a calculated instance:
  seq:
    - id: offset8
      type: u8
      if:  '_root.f_version > 1000000'
    - id: offset4
      type: u4
      if:  '_root.f_version <= 1000000'
  instances:
    offset:
      value: '_root.f_version <= 1000000 ? offset4 : offset8'
  1. Calculate the distinct switching variable using a calculated instance, then switch over it:
  seq:
    - id: offset
      type:
        switch-on: offset_type
        cases:
          4: u4
          8: u8
  instances:
    offset_type:
      value: '_root.f_version <= 1000000 ? 4 : 8'

I personally like second solution better. If you want to go fancy there, you can declare a enum of possible offset_type values and switch over them.

@GreyCat GreyCat self-assigned this Feb 4, 2017
@GreyCat GreyCat changed the title [question] How to make type of a field depend on field in another variable How to make type of a field depend on field in another variable Feb 4, 2017
@KOLANICH
Copy link
Author

KOLANICH commented Feb 4, 2017

Thank you for the answer.
1 when I have added _ it worked as I have written (for JS since it is a dynamic-typed language, but it will clearly fail for C++).
2 when I've done the case with

instances:
    offset_type:
      value: '_root.f_version <= 1000000 ? 4 : 8'

I got

KS compilation error:  java.lang.RuntimeException: can't do SwitchType(Name(identifier(offset_type)),Map(IntNum(4) -> IntMultiType(false,Width4,be), IntNum(8) -> IntMultiType(false,Width8,be))) Sub IntMultiType(false,Width4,be)

The first solution works.
3 can cases be upgraded in the following way:
a) if switch-on is present the value in it should be evaluated;
b) if switch-on is missing, every statement in cases acts the same as statement in if, the iteration stops on first true statement;
c) if becomes unnecessary and can be removed or repurposed;
d) cases applied to type utilizes union keyword of C for more effective storage
?

@GreyCat
Copy link
Member

GreyCat commented Feb 4, 2017

I've just tried exactly this .ksy:

meta:
  id: foo
  endian: le
seq:
  - id: f_version
    type: u4
  - id: my_offset
    type: offset_obj
types:
  offset_obj:
    seq:
      - id: offset
        type:
          switch-on: offset_type
          cases:
            4: u4
            8: u8
    instances:
      offset_type:
        value: '_root.f_version <= 1000000 ? 4 : 8'

It compiles without errors. Does it work for you?

3 can cases be upgraded in the following way:

I don't really like the idea of having a syntax where semantics of the same cases keys changes a lot due to presence of absence of some other key. In particular, it will make #27 much harder to implement.

Actually, if we'll go back to #10, there were some purposes to do if-like enhancements for switch. We might as well revisit these ideas.

d) cases applied to type utilizes union keyword of C for more effective storage

That's generally a bad idea from performance POV.

@KOLANICH
Copy link
Author

KOLANICH commented Feb 4, 2017

It compiles without errors. Does it work for you?

Yes, it does. But it doesn't if I insert your type into types. Here is the file you can use to debug the case.

That's generally a bad idea from performance POV.

Yes, it's true for the cases of small variables.

@mbrudka
Copy link

mbrudka commented Apr 30, 2018

Did you notice that the order of evaluation matters, for example:

        value2: '_root.value1 * 2'
        value1: '_root.f_version <= 1000000 ? 4 : 8'

What about recursive calls, like

        value1: '_root.value2 * 2'
        value2: '_root.value1 * 2' ?

Are you sure, that KSC can (easily) implement such syntax for expressions in all supported languages?

@GreyCat
Copy link
Member

GreyCat commented Apr 30, 2018

@mbrudka There's no magic. "Order of evaluation" problem is (sort of) solved by the fact that all instances are lazy by default and order of evaluation would be determined in runtime. Attempting to do infinitely recursive things will break due to inability to infer types.

@mbrudka
Copy link

mbrudka commented Apr 30, 2018

@GreyCat. Certainly, all described problems are solvable, but at the price of complexity. My proposition is just to keep KaitaiStruct simple without any additional contracts on the ways and results of evaluation. Let the developer of the protocol do some stuff, code the application and ensure that the frame is semantically proper.
I'd love to see serialization/building in KS, but similiar issues will shift this feature into quite distant future. I believe, it's better to have it quite fast then to design the additional semantic functions (in Construct these are validators, adapters, computables etc).

@GreyCat
Copy link
Member

GreyCat commented Oct 21, 2018

It looks like all the answers have been given, no further discussion occurred. Closing, feel free to reopen if any questions still remain.

@GreyCat GreyCat closed this as completed Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants