crystal-lang / crystal_lib

Automatic binding generator for native libraries in Crystal
138 stars 30 forks source link

IncompleteArrayType spec fails due to LLVM 3.9 backwards incompatible changes #40

Closed mverzilli closed 7 years ago

mverzilli commented 7 years ago

Env: macOS 10.12.2, LLVM 3.9.1

This test is failing with LLVM 3.9: https://github.com/crystal-lang/crystal_lib/blob/029bd721a0d5107ba9a2140fe9c9780988f2e454/spec/parser_spec.cr#L237

This is a reduced snippet to reproduce the issue:

require "../src/crystal_lib"
require "../src/clang"

include CrystalLib

nodes = Parser.parse("int some_var[];")
var = nodes.last.as(Var)
pp var

When linking against LLVM 3.7, the code above yields:

var # => #<CrystalLib::Var:0x10feca4e0
          @name="some_var",
          @type=
           #<CrystalLib::IncompleteArrayType:0x10feca500
            @type=#<CrystalLib::PrimitiveType:0x10ee4dfc0 @kind=Int>>>

Whereas linking against LLVM 3.9, it yields:

var # => #<CrystalLib::Var:0x10327a4a0
          @name="some_var",
          @type=
           #<CrystalLib::ConstantArrayType:0x10327a4c0
            @size=1,
            @type=#<CrystalLib::PrimitiveType:0x1029bbfc0 @kind=Int>>>

I'll be happy to send in a PR to fix the test, but I'm not sure what's the right way to go. If I fix it for 3.9, it'll fail on 3.7.

I could put the test behind an {% if %} that checks for the current LLVM version and tests conditionally, but would that make sense? Instead of doing that, I guess it'd make more sense simply to remove the test and trust whatever type the underlying clang version tells us to assign to that expression.

cc: @asterite @spalladino @ysbaddaden

ysbaddaden commented 7 years ago

Some changes were introduced in LLVM 3.8 or 3.9, where there are now opaque EnumType and RecordType, which break how crystal_lib is detecting an Enum type or a Struct definition. I understood the issue, but don't know how to start fixing that for crystal_lib —it seems to rely too much on type when it should rely more on cursors, then seek for types, but I must be mistaken.

mverzilli commented 7 years ago

We just removed that spec. It didn't make sense to maintain it because anyway we are delegating the responsibility to Clang. So this is fixed (as in, now the specs pass).