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

object.c: rb_eql returns int not VALUE #6508

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Conversation

casperisfine
Copy link
Contributor

It works, but assumes Qfalse == 0, which is true today but might not be forever.

Copy link
Member

@nobu nobu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems reasonable, but I'm strongly against changing Qfalse itself.

@rhenium
Copy link
Member

rhenium commented Oct 7, 2022

The doc comment in include/ruby/internal/intern/object.h needs to be updated to reflect this change.

Apparently, rb_eql() actually used RTEST() before commit b827fdf (2017, Ruby 2.5).

@casperisfine
Copy link
Contributor Author

I'm strongly against changing Qfalse itself.

That change is not in order yet and would be complicated, but we may want to do it one day: https://bugs.ruby-lang.org/issues/18397

The main reason for the change here is the incompatible types. I just saw this while looking at something else and was really surprised.

The doc comment in include/ruby/internal/intern/object.h needs to be updated to reflect this change.

Oh wow:

/**
 * Checks for equality of the passed objects, in terms of `Object#eql?`.
 *
 * @param[in]  lhs          Comparison left hand side.
 * @param[in]  rhs          Comparison right hand side.
 * @retval     RUBY_Qtrue   They are equal.
 * @retval     RUBY_Qfalse  Otherwise.
 * @note       This  function  actually  calls `lhs.eql?(rhs)`  so  you  cannot
 *             implement your class' `#eql?` method using it.
 */
int rb_eql(VALUE lhs, VALUE rhs);

This doc make little sense to me. And to be fair I don't understand why int rb_eql vs VALUE rb_equal.

Was this function meant to be VALUE rb_eql or something?

Should I open an issue for this to be calrified?

@casperisfine
Copy link
Contributor Author

Ok, so this documentation was only added very recently in d43acca

So I think explicitly documenting Qfalse / Qtrue wasn't particularly intended. I'll update the documentation.

@nobu
Copy link
Member

nobu commented Oct 8, 2022

Non-zero feels better than 1, to not expect it too restricted.

It works, but assumes `Qfalse == 0`, which is true today
but might not be forever.
@byroot byroot merged commit 1a7e7bb into ruby:master Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants