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

Address FIXME #8368

Merged
merged 2 commits into from Oct 29, 2019
Merged

Address FIXME #8368

merged 2 commits into from Oct 29, 2019

Conversation

vlazar
Copy link
Contributor

@vlazar vlazar commented Oct 23, 2019

I've noticed a FIXME comment intended to be addressed once the issue #8015 is fixed. It is fixed now by #8019.

@Blacksmoke16
Copy link
Member

Wouldn't this break with Nil since there isn't any logic handling it now? The FIXME said to merge it with the previous conditional, not to remove it entirely.

@vlazar
Copy link
Contributor Author

vlazar commented Oct 23, 2019

@Blacksmoke16 These are the cases I've checked locally:

p! Pointer(Void).null.as(Nil) # => nil
p! nil.as(Void*).as(Nil) # => nil
a = 1_i64
p! pointerof(a).as(Nil).nil? ? 3 : 7 # => 3

First two from #8015 (comment)
The third one from this spec https://github.com/crystal-lang/crystal/pull/8019/files#diff-c4efe0ebc6736f5e9dc5804721a547f6R68

Also checked these https://github.com/crystal-lang/crystal/pull/8016/files#diff-277dafebcd586c8b31a0f852df13a6edR10-R23

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Oct 23, 2019

I don't know enough about this to know if its an issue or not. I was just pointing out that Nil gets handled by the else block now, versus the if block.

https://play.crystal-lang.org/#/r/7uyr

Using that example it seems to return nil just fine either way so 🤷‍♂ , I'll let someone else weigh in.

@vlazar
Copy link
Contributor Author

vlazar commented Oct 23, 2019

@Blacksmoke16 Thank you for pointing this out! I think there is a tiny chance (99.999...%) you are right and I should have done this instead:

  def self.unbox(pointer : Void*) : T
     {% if T <= Reference || T == Nil %}
       pointer.as(T)
     {% else %}
       pointer.as(self).object
     {% end %}
  end

@asterite asterite added this to the 0.32.0 milestone Oct 29, 2019
@asterite asterite merged commit 3c3647d into crystal-lang:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants