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

Value Instance does not accept type attribute (despite the documentation's assertion) #463

Closed
webbnh opened this issue Aug 2, 2018 · 6 comments
Milestone

Comments

@webbnh
Copy link

webbnh commented Aug 2, 2018

[My apologies if this has already been reported -- the obvious search terms turned up more issues than I could check -- maybe I'm just feeble.]

I have a pile of KSY files. They contain a variety of type definitions, but they all have a few which are common, where "common" means, same name and fields with the same names (but, possibly in a different order or with different sizes, etc.). In particular, one type definition has a "value" instance, payload_size. However, in some of my KSY files, the return type for the getter function for the value is signed while in others it is unsigned. I can't use conditional code to access them, and I'd prefer to avoid a typecast in my source (I'm in C++), so, I would like to specify a type attribute for the instance, which the the KSY Reference documentation suggests I can do:

Instance specification is very close to Attribute spec (and inherits all its properties), but it specifies an attribute that lies beyond regular parsing sequence. Typically, each instance is compiled into a lazy reader function/method that will parse (or calculate) requested data on demand, cache the result and return whatever’s been parsed previously on subsequent calls.

Everything that described in Attribute spec can be used, except for id, which is useless, because all instances already have name due to map string key.

(Emphasis mine.)

However, the compiler disagrees:

/types/record/instances/payload_size/type: invalid key found in value instance, allowed: doc, doc-ref, enum, if, value

So...which is one wrong, the compiler or the documentation? :-)

(While you're pondering that, is there a way that I can use KS's typecasting to work around the problem? Can I cast the result to a u2? If so, what's the syntax for that? Thanks!)

@KOLANICH
Copy link

KOLANICH commented Aug 2, 2018

Value Instance does not accept type attribute (despite the documentation's assertion)

There is a pretty bad workaround:

instances:
  a:
    pos: 0
    type: "b(c)"

Unfortunately it won't help you, you can't convert a value instance into a user-controlled-size built-in type (except bool) for now. Internally nearly all the integer value instances are CalcIntType. And they are transformed into a 32-bit int (BTW, @GreyCat, why not 64-bit?)

See #127 about it.

@GreyCat
Copy link
Member

GreyCat commented Aug 4, 2018

Unfortunately, KSY reference is very outdated and needs major revamp. "Instance spec" it describes is actualy a "parse instance", not "value instance".

This question was indeed raised in several different contexts quite a few times before:

This particular problem is somewhat more sophisticated than you have might expect. Namely, if we'll be talking about a + b expression, if a is of s4 type and b is of u8 type, what would be the type of resulting integer?

The simple answer is: we can't tell, as it is very dependent on target-language (and, sometimes, even implementation-dependent).

KS expressions are built with this in mind, introducing CalcIntType for that, which is "your average one size fits all most" integer.

It looks like that what you want in your particular case is indeed typecasting, not "setting a type of value instance" (as this is happening automatically). Typecasting would have been used as (a + b).as<u8>, but, as far as I remember, current implementation only allows typecasting to user types. This is actually not that hard to fix, given that we've already introduced so called "pure types" for the purpose for type: designation in params.

@GreyCat
Copy link
Member

GreyCat commented Aug 4, 2018

Actually, I've implemented it right now. Stuff like

(a + b).as<u2>

should yield

static_cast<uint16_t>(a + b)

in C++.

@webbnh
Copy link
Author

webbnh commented Aug 4, 2018

Thanks for the historical references (clearly, you are better at working the search mechanism than I am!).

As for the ambiguity of the type resulting from the expression, that's the exact problem that I was hoping to solve by specifying the type attribute. However, the problem that it would solve is only on my side of the interface. On your side, you would still need to use type derivation to determine whether a typecast was required (or, I suppose, you could emit one unconditionally and let the (in my case) C++ compiler deal with it).

Using a typecast in the instance value expression will suffice. (I had tried it, and it was clear that it didn't work for primitive types.) Thanks for implementing it so quickly! Any clue when it will be released? :-)

@GreyCat
Copy link
Member

GreyCat commented Aug 4, 2018

You can use unstable releases right away. We publish packages for Debian and Windows.

@GreyCat GreyCat added this to the v0.9 milestone Mar 28, 2019
@GreyCat
Copy link
Member

GreyCat commented Mar 28, 2019

Problem seems to be fixed, will be part of 0.9 release.

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

No branches or pull requests

3 participants