Fix parsing and behaviour of ->foo.[] and other operators
#7334
Conversation
e9fd826 to
ec6a8ea
Compare
|
@bcardiff It seems something overflows in the codegen debug code... |
|
@asterite yup debugging in CI since nightly also failed, it seems |
|
@bcardiff The plot thickens :-) But I can't find anywhere where |
|
It's https://github.com/crystal-lang/crystal/blob/master/src/lib_c/x86_64-linux-gnu/c/sys/types.cr#L45-L49 that is required in the prelude , now I need to CI to fail again in my fork 👷 🛠 ... |
| @@ -366,8 +366,8 @@ class Crystal::CodeGenVisitor | |||
| end | |||
|
|
|||
| private def codegen_raise_overflow | |||
| location = @call_location.not_nil! | |||
| set_current_debug_location(location) if @debug.line_numbers? | |||
| location = @call_location | |||
There was a problem hiding this comment.
Using #not_nil! is intentional here. Having debug location information is mandatory for when debug information is wanted to be emitted. Otherwise the program will fail with --debug but run ok, so it was to catch that missing information even when not using --debug.
There was a problem hiding this comment.
That makes sense. However, with this PR this code can now be triggered when there's no call at all. For example:
a = 1
f = ->a.+(Int32) # this ends up generation a function that implements + and can raise overflow
f.call(Int32::MAX)The ->a.+(Int32) can happen multiple times in code so there's no single "call" to use, unlike when you write a + 1. I don't know how to handle this...
There was a problem hiding this comment.
Can't we reach the location of the proc literal when inlining the primitive?
There was a problem hiding this comment.
We could. But that will not always work correctly because two proc literals that point to the same method will result in the same function being generated, so the location will point to the first literal, which is not always correct.
Maybe we need a reproducible test case to check for when this breaks.
There was a problem hiding this comment.
Oh, I see. Well the story needs to continue with more support for debugging information then.
There was a problem hiding this comment.
I'll see what I can do :-)
There was a problem hiding this comment.
I mean in a future PR, but if you want to tackle that now I wont interfere :-P
ec6a8ea to
e36200f
Compare
|
This seems good to 🚢 👍 |
|
@asterite Can you rebase this branch on master to make sure it's all still good? |
e36200f to
b54c77f
Compare
|
@straight-shoota Rebased and green. |
|
Hm, should the last commit be squashed and then merge three indivdual commits? |
b54c77f to
acdb36f
Compare
|
@straight-shoota Done |
|
Awesome! Thanks @asterite |
Fixes #7319
Split into several commits, but basically:
->foo.opwhereopwas[],+, etc. didn't work, so that's fixed+) are primitives and inlined by the compiler, and this didn't work when used as proc pointer. Another reason is that primitives are not passed by reference (pointer) and that's needed when the proc pointer closures over something (it's a pointer). Both of these things are fixed.IndexableThe code in this PR feels slightly hacky, but primitives are a special case in the language and so they usually need special code to handle them, so it's probably fine.