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

Fix StringLiteral#!=, SymbolLiteral#!= and MacroId#!= #2859

Merged
merged 1 commit into from
Jun 16, 2016

Conversation

jhass
Copy link
Member

@jhass jhass commented Jun 16, 2016

No description provided.

@jhass jhass added this to the 0.18.1 milestone Jun 16, 2016
@asterite
Copy link
Member

Merge? :-)

jhass added a commit that referenced this pull request Jun 16, 2016
Fix StringLiteral#!=, SymbolLiteral#!= and MacroId#!=
@jhass jhass merged commit 2a75080 into crystal-lang:release/0.18 Jun 16, 2016
@jhass
Copy link
Member Author

jhass commented Jun 16, 2016

Yup :)

@jhass jhass deleted the string_literal_equals branch June 16, 2016 23:58
@jkthorne
Copy link
Contributor

Have you thought about using the original case statement to determine the decision tree?
It seems unintuitive to me to have a case statement on the "method" and then an additional case statement in the inner block.

I thought it might be easier to read if it was broken out into 2 conditionals.
So changing it from this.

      case method
      when "==", "!="
        case arg = args.first?
        when MacroId
          if method == "=="
            return BoolLiteral.new(@value == arg.value)
          else
            return BoolLiteral.new(@value != arg.value)
          end
        else
          return super
        end

to something like this

      case method
      when "=="
        if MacroId
          return BoolLiteral.new(@value == args.first.value)
        else
          return super
        end
      when "!="
        if MacroId
          return BoolLiteral.new(@value != args.first.value)
        else
          return super
        end

@jhass
Copy link
Member Author

jhass commented Jun 17, 2016

Looks like about the same amount of boilerplate to me ;)

@jkthorne
Copy link
Contributor

the lines of code are the same it was just about moving the check for the string "==" and "!=" to the outer cases statement so there are no inner checks.

Lines of code the same.

But I guess it made it easier for me to think about.

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

3 participants