-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[Truffle] undefined is now the same as NotProvided.INSTANCE. #4385
[Truffle] undefined is now the same as NotProvided.INSTANCE. #4385
Conversation
* The condition is impossible.
…y_convert_to_encoding.
…erter#initialize.
…ided.INSTANCE in Java. * "if undefined" and "undefined.call" are unsupported, on purpose. undefined arguments should be checked early and not carry the undefined around.
a714351
to
3bd4249
Compare
left = Rubinius::Type.coerce_to_collection_length one | ||
left += size if left < 0 | ||
left = 0 if left < 0 | ||
|
||
if two and !undefined.equal?(two) | ||
if !undefined.equal?(two) and two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why and
rather than &&
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just used to and
over &&
as it looks less cryptic and most of the time there is no need for the higher precedence of &&
. But either are fine. The original code used and
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik: Original intention is that &&
is a logical operator and and
is a control flow operator. They are not meant to be interchangeable. Following is interchangeable, although the first is much more common.
return :nothing if test.nil?
test.nil? and return :nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my opinion: second one looks like Perl or PHP and and
is needed for assignment in conditionals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convention is to add parenthesis if (a = true) && b.nil?
to make the assignment stand out more. I like test or raise something
form since it puts the test first not the unimportant exception as in raise something unless test
. Anyway this is not important, it's opinionated and gain is just minor readability improvements.
7ac33a3
to
3bd4249
Compare
@@ -278,7 +279,7 @@ | |||
private final GlobalVariables globalVariables; | |||
private final DynamicObject mainObject; | |||
private final DynamicObject nilObject; | |||
private final DynamicObject rubiniusUndefined; | |||
private final Object rubiniusUndefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably out of scope, but I think we can rename this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, some cleanup can be done after, since we can get rid of this whole field and guard.
@@ -53,9 +53,10 @@ class Complex < Numeric | |||
undef_method :i | |||
|
|||
def self.convert(real, imag = undefined) | |||
if real.equal?(nil) || imag.equal?(nil) | |||
if nil.equal?(real) || nil.equal?(imag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you modified the order here, is there any reason to not just use real.nil?
and imag.nil?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they might be undefined
, and calling methods on undefined
does not work, on purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though calling literal.equal?(some_var)
is less nice, it's actually better for the semantics, otherwise you could invoke a method on a user object (more important for ==
than equal?
which is rarely redefined). We could play some tricks in core, but I'd rather avoid too much magic unless needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. This seems like a rule someone (probably me) is bound to forget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there are a few places in core where it's not like this.
For undefined
you can't forget as it will trigger an assert if you write it in the other way :)
@@ -141,7 +141,7 @@ class Converter | |||
def self.asciicompat_encoding(string_or_encoding) | |||
encoding = Rubinius::Type.try_convert_to_encoding string_or_encoding | |||
|
|||
return if not encoding or undefined.equal? encoding | |||
return unless encoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't BooleanCastNode
need to be updated to handle the undefined
object in order for this to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of try_convert_to_encoding was changed to avoid this precisely :)
false
is returned where undefined
was before in that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined
used to be "truthy" so I did not dare to accept it in BooleanCastNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. I would have guessed it was "falsey."
@@ -588,13 +588,12 @@ def self.foreach(name, separator=undefined, limit=undefined, options=undefined) | |||
|
|||
name = Rubinius::Type.coerce_to_path name | |||
|
|||
separator = $/ if undefined.equal?(separator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably doesn't matter all that much, but this seems to do extra processing that we didn't have before and doesn't seem to make things all that much clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. I see this is the case
issue you mentioned in the PR description.
@jruby/truffle Please share your opinion.
I think the code is in general better with these changes,
undefined
can't get away and can only be used as a marker for "not provided argument" which is the main purpose.The only case which might be worth supporting would be to allow it is
case
statements, which could be done more easily with something like:and then Undefined would have a special
===
that only matches the right instance.I don't think it's worth it, usually logic is just duplicated for the undefined case, so it might as well be normalized before, with
arg = sensible_default if undefined.equal?(arg)