iets3.opensource icon indicating copy to clipboard operation
iets3.opensource copied to clipboard

Support hexadecimal number literals and hexadecimal number ranges

Open alexanderpann opened this issue 2 years ago • 7 comments

The hexadecimal numbers work similar to the ones in baselanguage. They are considered integers and also support negative values. Internally, they are converted to decimal whenever they are used, that means that the typesystem also converts them to decimal. They are only displayed in hexadecimal form in the editor and the left side of the typesystem debug tool. Hex values are also now supported in NumberRangeSpecs e.g., [-0xff | 0xff].

Additional fixes:

  • NumberRangeSpec now validates through the NumberLiteral class.
  • Precision expression for integers are now not only supported in the generator but also in the interpreter.

Example:

Screenshot 2022-12-13 at 12 41 33

alexanderpann avatar Dec 13 '22 11:12 alexanderpann

@alexanderpann is there an easy way to exclude this from projects which don't work with hex numbers? This seems rather specific for me. Based on your description, the hex numbers are integrated in the usual number types. I'd suggest to have this in a separate concept, ideally in a separate language and plugin and ship this as optional add on. Since this feature was not requested in multiple projects in over 5 years of KernelF, I think it's dangerous to "force" this in the customer projects by adding it to the existing number types/ Languages.

lhartl avatar Dec 21 '22 14:12 lhartl

@lhartl In baselanguage it is indeed an extra literal that extends IntegerLiteral. I don't see it that way in KernelF as it is just another way of entering an integer number (in base 16 instead of base 10 to be precise). We don't even want to generate different code for it. The value is automatically converted to base 10 when needed (the typesystem also sees it as a normal integer), as it is done in getCompileTimeConstantValue of the HexIntegerLiteral in baselanguage. Do you really want to change many places to support the HexIntegerLiteral concept instead of seeing it a special notation of the IntegerLiteral? For example, changing the interpreter and generator? We could make the hex literals opt-in through the extension point primitiveTypeMapper or another extension point if you prefer it that way. As long as you don't use them, nothing will break. Every change in this PR is meant to be backward compatible.

alexanderpann avatar Dec 21 '22 14:12 alexanderpann

The implementation itself is nice and using the constraints to allow additional values (hex ones) is a good non breaking approach. However the main problem I see here is that as soon as users are allowed to use the new hex values, their old and not updated implementation (in calculations, generators etc.) would break. So introducing this feature "explicetly" and on purpose would be really important here. Meaning we would need to give customers the posibility to:

  1. update their impl. using min/max with hex values first
  2. then give them the possibility to "switch on" the new feature

IMHO shipping it "under the hood" is not a good approach and might bring more trouble then value.

arimer avatar Apr 12 '23 10:04 arimer

I totally agree with @arimer, we can not silently ship this without supporting the feature in the generators. This might not only affect itemis team. We need coordination on this. could you please hold back the merge until we figured out how to handle this?

lhartl avatar Apr 12 '23 13:04 lhartl

@HeikoBecker any progresson this one?

arimer avatar Jun 14 '23 13:06 arimer

@HeikoBecker any progresson this one?

No, this PR is currently blocked.

HeikoBecker avatar Jun 21 '23 06:06 HeikoBecker

@HeikoBecker, thx for the update. I converted it to a draft PR. In case you continue working on it, feel free to change the status.

arimer avatar Jun 21 '23 06:06 arimer

Closing this PR in agreement with @alexanderpann since it is now quite outdated.

Edit: Should not be closed for now. Might still be relevant. Sorry for the noise.

HeikoBecker avatar Sep 18 '24 06:09 HeikoBecker

Let's close it. I'll reimplement it. I already have a better idea how to not make this a breaking change.

alexanderpann avatar Sep 25 '24 06:09 alexanderpann