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

Extend ordering comparisons (< , <=, etc) to Bool & Character #2430

Merged
merged 29 commits into from
Apr 19, 2023

Conversation

darkdrag00nv2
Copy link
Contributor

@darkdrag00nv2 darkdrag00nv2 commented Apr 9, 2023

Work towards #2276

Description

This PR does the following:

  • Extracts comparison related interface methods to a new interface ComparableValue
  • Adds IsComparable to the type interface
  • Updates the checker to use the IsComparable method to detemine comparability
  • Implements the comparison support for Bool and Character

Support for String can be done as a follow up.

TODOs before this is ready for review:

  • Fix tests for numbers
  • Add tests for bool & character
  • Update documentation

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov
Copy link

codecov bot commented Apr 9, 2023

Codecov Report

Merging #2430 (213c480) into master (2bb742f) will decrease coverage by 0.23%.
The diff coverage is 48.08%.

@@            Coverage Diff             @@
##           master    #2430      +/-   ##
==========================================
- Coverage   78.52%   78.29%   -0.23%     
==========================================
  Files         315      325      +10     
  Lines       68655    72464    +3809     
==========================================
+ Hits        53908    56737    +2829     
- Misses      12942    13644     +702     
- Partials     1805     2083     +278     
Flag Coverage Δ
unittests 78.29% <48.08%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/sema/block.gen.go 100.00% <ø> (ø)
runtime/sema/character.gen.go 100.00% <ø> (ø)
runtime/sema/deployedcontract.gen.go 100.00% <ø> (ø)
runtime/sema/meta_type.go 100.00% <ø> (ø)
runtime/sema/path_type.go 100.00% <ø> (ø)
runtime/sema/storable_type.go 100.00% <ø> (ø)
runtime/sema/string_type.go 95.87% <ø> (ø)
runtime/sema/type.go 89.32% <13.33%> (-0.59%) ⬇️
runtime/interpreter/value.go 69.57% <35.00%> (-0.02%) ⬇️
runtime/sema/gen/main.go 89.34% <44.44%> (-0.38%) ⬇️
... and 3 more

... and 26 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bluesign
Copy link
Contributor

bluesign commented Apr 9, 2023

does comparable make sense for Bool ? I don't have much CS background but kind of makes no sense to me.

@darkdrag00nv2
Copy link
Contributor Author

@bluesign Yeah, I am not 100% on its usefulness. However, Kotlin does support comparison for booleans. See the discussion on the issue #2276.

@bluesign
Copy link
Contributor

bluesign commented Apr 10, 2023

I think it is a bit problematic; ( more headache )

a>true -> meaningless  ( prone to mistake, probably need to reject anyway ) 
a<false -> meaningless ( prone to mistake, probably need to reject anyway ) 
a>=true -> a==true
a<=false -> a==false 
a>false  -> a==true
a<true  -> a==false 

basically anything we compare with a constant, should be handled by ==

Some can say maybe it can be useful for sorting array of anystruct or something, but I think gain is minimal there. ( as we will not be able to compare false to 42 for example )

@darkdrag00nv2
Copy link
Contributor Author

@turbolent Thoughts on the above comment?

Also add tests and documentation
@darkdrag00nv2 darkdrag00nv2 changed the title Extend ordering comparisons (< , <=, etc) to Bool Extend ordering comparisons (< , <=, etc) to Bool & Character Apr 10, 2023
@darkdrag00nv2 darkdrag00nv2 marked this pull request as ready for review April 10, 2023 20:51
@darkdrag00nv2
Copy link
Contributor Author

Still need to add tests for the execution logic but opened it up for review for feedback.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/sema/check_binary_expression.go Outdated Show resolved Hide resolved
runtime/sema/check_binary_expression.go Outdated Show resolved Hide resolved
@turbolent turbolent self-assigned this Apr 10, 2023
darkdrag00nv2 and others added 4 commits April 11, 2023 19:49
Accept suggestion to unnest

Co-authored-by: Bastian Müller <bastian@turbolent.com>
Update comment.

Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/sema/check_binary_expression.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/sema/check_binary_expression.go Outdated Show resolved Hide resolved
darkdrag00nv2 and others added 8 commits April 13, 2023 23:43
Co-authored-by: Daniel Sainati <sainatidaniel@gmail.com>
Co-authored-by: Daniel Sainati <sainatidaniel@gmail.com>
Co-authored-by: Daniel Sainati <sainatidaniel@gmail.com>
Co-authored-by: Daniel Sainati <sainatidaniel@gmail.com>
@darkdrag00nv2
Copy link
Contributor Author

Update on the PR:

  • Resolved the review suggestions
  • Added interpreter tests for char and bool
  • Included upper and lowercase character comparison example in the documentation

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

@turbolent
Copy link
Member

@SupunS Could you also please have a look when you have time? Thanks!

@darkdrag00nv2
Copy link
Contributor Author

@SupunS Gentle reminder 🙂

@turbolent
Copy link
Member

@dsainati1 Could you also please review the latest changes?

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Looks good! Just one question and a nitpick.

Sorry for taking so long for the review! Appreciate the patience 🙏

runtime/sema/check_binary_expression.go Outdated Show resolved Hide resolved
@turbolent turbolent merged commit 09c7ae2 into onflow:master Apr 19, 2023
9 of 14 checks passed
@darkdrag00nv2 darkdrag00nv2 deleted the ordering_containers branch April 20, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants