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

Use TypeNode#union_types where possible #5018

Closed
wants to merge 2 commits into from

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Sep 22, 2017

Follow-up to #4995.

Meant to be merged after v0.24.0 release.
All specs are passing on current master.

@bew
Copy link
Contributor

bew commented Sep 22, 2017

Also in primitives.cr

crystal/src/primitives.cr

Lines 270 to 272 in b52b2f9

{% ints = %w(Int8 Int16 Int32 Int64 UInt8 UInt16 UInt32 UInt64) %}
{% floats = %w(Float32 Float64) %}
{% nums = %w(Int8 Int16 Int32 Int64 UInt8 UInt16 UInt32 UInt64 Float32 Float64) %}

And io/byte_format.cr
{% for type, i in %w(Int8 UInt8 Int16 UInt16 Int32 UInt32 Int64 UInt64) %}

@Sija
Copy link
Contributor Author

Sija commented Sep 22, 2017

@bew I thought it wouldn't compile, but it does, thanks for pointing out!

@Sija
Copy link
Contributor Author

Sija commented Sep 22, 2017

@bew io/byte_format.cr is required by prelude so can't do, or at least I dunno how.

@bew
Copy link
Contributor

bew commented Sep 22, 2017

I don't see the possible issue with prelude, can you explain?

@Sija
Copy link
Contributor Author

Sija commented Sep 22, 2017

Compiler error will say more than me ;)

Using compiled compiler at `.build/crystal'
Error in line 1: while requiring "prelude"

in src/prelude.cr:19: while requiring "string"

require "string"
^

in src/string.cr:4235: while requiring "./string/builder"

require "./string/builder"
^

in src/string/builder.cr:1: while requiring "io"

require "io"
^

in src/io.cr:1142: while requiring "./io/*"

require "./io/*"
^

in src/io/byte_format.cr:123: expanding macro

  {% for mod in %w(LittleEndian BigEndian) %}
  ^

in src/io/byte_format.cr:125: undefined constant Int::Primitive

      {% for type, i in Int::Primitive.union_types %}
                        ^~~~~~~~~~~~~~

@bew
Copy link
Contributor

bew commented Sep 22, 2017

It might work to move require "int" above require "string" in prelude.cr, but I'm not sure it's how we should do it..
@asterite will know better I think! 😃

@RX14
Copy link
Contributor

RX14 commented Sep 22, 2017

This should be merged after the next release, as it depends on a new compiler feature.

@Sija
Copy link
Contributor Author

Sija commented Mar 6, 2018

I dunno how to fix it, I'll close it unless there's someone willing to help.

@bew
Copy link
Contributor

bew commented Mar 6, 2018

Did you tried my suggestion?

@Sija
Copy link
Contributor Author

Sija commented Mar 6, 2018

@bew yes, it doesn't work.

@miketheman
Copy link
Contributor

#codetriage

Rebasing this PR on top of master:

$ bin/crystal --version
Using compiled compiler at `.build/crystal'
Crystal 0.26.1+36 [bc526468e] (2018-09-08)

LLVM: 6.0.1
Default target: x86_64-apple-macosx

Running make std_spec produces this output:

  1) JSON serialization from_json deserializes union with Int128 (fast path)
     Failure/Error: {% for type in Int::Primitive.union_types %}

       Expected: 170141183460469231731687303715884105727_i128
            got: -1_i128

     # spec/std/json/serialization_spec.cr:155

make compiler_spec fails a LOT, all around code generation. Example:

Codegen: super calls super with dispatch (#2318)

       while requiring "primitives" (Crystal::TypeException)
         from src/compiler/crystal/semantic/ast.cr:13:9 in 'raise'
         from src/compiler/crystal/semantic/ast.cr:12:5 in 'raise'
         from src/compiler/crystal/semantic/semantic_visitor.cr:72:5 in 'visit'
         from src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
         from src/compiler/crystal/semantic/top_level_visitor.cr:707:9 in 'visit'
         from src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
         from src/compiler/crystal/semantic.cr:62:7 in 'top_level_semantic'
         from src/compiler/crystal/semantic.cr:22:23 in 'semantic'
         from src/compiler/crystal/semantic.cr:21:3 in 'semantic'
         from src/compiler/crystal/codegen/codegen.cr:22:7 in 'run'
         from spec/spec_helper.cr:184:5 in 'run'
         from spec/spec_helper.cr:156:1 in 'run'
         from spec/compiler/codegen/super_spec.cr:396:5 in '->'
         from src/spec/methods.cr:255:3 in 'it'
         from spec/compiler/codegen/super_spec.cr:395:3 in '->'
         from src/spec/context.cr:255:3 in 'describe'
         from src/spec/methods.cr:16:5 in 'describe'
         from spec/compiler/codegen/thread_local_spec.cr:1:1 in '__crystal_main'
         from src/crystal/main.cr:97:5 in 'main_user_code'
         from src/crystal/main.cr:86:7 in 'main'
         from src/crystal/main.cr:106:3 in 'main'

I'm not entirely sure how to proceed here.

@HertzDevil
Copy link
Contributor

This amounts to adding 128-bit integer support to the affected methods, so this cannot be done now. Specs however can opt in to BUILTIN_INTEGER_TYPES from spec/support/number.cr.

@Sija
Copy link
Contributor Author

Sija commented Oct 20, 2022

Closing in favor of #12636

@Sija Sija closed this Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants