Numeric literal suffixes #10493
Numeric literal suffixes #10493
Conversation
|
I think this should be implemented by adding new constructors to |
|
The I'm not sure about the |
|
I originally was thinking of having the suffix in tconstant and using some sort of filter to generate the Int64 make and UInt casts but decided against it as you'd just end up with Managed to work around the i64 reification issue by manually constructing the |
|
I've added support for the integer suffixes on hex literals, also if you append i64 to a hex literal it now allows up to 16 hex characters as opposed to 8 for other suffixes or un-suffixed hex literals (closes #5150). As for parseInt / parseFloat I don't think it makes sense to support suffixes in the input strings as you can't change the output type of parseInt if you input |
| @@ -568,11 +568,11 @@ let rec constructor_side_effects e = | |||
|
|
|||
| let type_constant basic c p = | |||
| match c with | |||
| | Int s -> | |||
| | Int (s,_) -> | |||
There was a problem hiding this comment.
I don't think this function should ignore the suffixes.
There was a problem hiding this comment.
Yeah, wasn't sure what to do here since currently the suffixes are handled just before (mainly for the simplicity of creating the int64 make and uint cast with the untyped ast).
Would it be better to try and manually create the texprs here instead?
There was a problem hiding this comment.
Hmm, this situation makes me think that we should consider dealing with the tconstant part of this as well, but that would add some complexity to everything...
Generally, I haven't had great experiences with syntactic rewrites like this. However, I acknowledge that it's very convenient here.
Ultimately, we're going to need some sort of mk_int64 : int64 -> texpr function that doesn't require the full typer context. Which is already a problem because Int64.make is a pretty high-level function that relies on inline. I'll have to think about how to approach this.
|
I propose that we merge this now and then look into our internal handling of literals as a second step. The changes here shouldn't break anything, at worst there's some part that doesn't work correctly yet. Could you resolve the conflict? |
| @@ -111,6 +111,25 @@ let is_valid_identifier s = | |||
| with Exit -> | |||
| false | |||
|
|
|||
| let split_int_suffix s = | |||
| let is_signed = String.contains s 'i' in | |||
| match String.index_opt s (if is_signed then 'i' else 'u') with | |||
There was a problem hiding this comment.
Wouldn't try String.index show better performance? I think numeric constants are quite numerous in hx sources.
There was a problem hiding this comment.
This is unlikely to ever be measurable, and it's not clear if the alternative is actually better.
There was a problem hiding this comment.
Guess the match could be changed to something like match (try String.index s 'i' with _ -> (try String.index s 'u' with _ -> -1)) with ... which is less readable at a glance for me, not sure if any performance difference would really be noticable to make it worthwhile
src/typing/typeload.ml
Outdated
| @@ -345,8 +345,8 @@ let rec load_instance' ctx (t,p) allow_no_params = | |||
| | TPExpr e -> | |||
| let name = (match fst e with | |||
| | EConst (String(s,_)) -> "S" ^ s | |||
| | EConst (Int i) -> "I" ^ i | |||
| | EConst (Float f) -> "F" ^ f | |||
| | EConst (Int (i,_)) -> "I" ^ i | |||
There was a problem hiding this comment.
This may lead to misses for types like MyType<123i32> and MyType<123i64>
There was a problem hiding this comment.
Good point, we can just append the suffix here.
There was a problem hiding this comment.
Actually this makes me realize that we might have to sanitize the suffixes somewhere if they aren't typed as expressions. I think we could sneak in invalid strings via macros otherwise.
There was a problem hiding this comment.
ah, yes, I think I see what you mean. macro : MyType<123i40> allows the i40 suffix into the type parameter (I'm currently ignoring the suffix in that part of the reify since I wasn't sure when it was called until now).
I'll look into not ignoring the suffixes in this case and adding some sort of check, not sure if there are any other places you could add invalid suffixes as well.
There was a problem hiding this comment.
I've added a check in macro api decoding to ensure the suffixes of any created numeric constants are valid. Not sure if there's a better place to put these checks so its not done both in the typer and at macro decoding.
|
Noticed that with the i32 suffix literals could still be changed to floats if they were too large for 32 bits. Changed it to error instead since changing to a float doesn't make much sense if you're explicitly using the i32 suffix. |
|
Closes #10481
Opening up a draft with my progress so far to make sure I'm not going off in the wrong direction.
Ast constant int and float expressions now contain an optional suffix string. During typing the suffix is inspected and in the case of
i32orf64the constant is passed through as is and withi64oru32an appropriate expression is inserted. This does mean that suffixes don't make it into the typed ast which I'm not sure is ideal but does simplify things.In many cases I've discarded the suffix or just inserted None to get it to compile, while things seem to work pretty well as is in my basic testing I'll have to go through and understand whats going on in many of those places and make sure its right. I've made a note of things which I know about / need to do below.