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

Don't use float equality for AST comparisons #1238

Merged
merged 22 commits into from
Feb 8, 2025

Conversation

Clyybber
Copy link
Contributor

@Clyybber Clyybber commented Mar 13, 2024

Summary

Float equality lacks the substitution nor reflexivity properties usually
expected from an equality operator, so it's not correct to use float
equality in AST comparisons. This PR changes it so that they are
compared for bit equality.

Details

  • 0.0 and -0.0 are not being considered equal by the compiler
    anymore, this affects:

    • static parameters in generic types and procedures, see tests/statictypes/tstatictypes.nim
    • default arguments for forward declarations, see tests/errmsgs/tforwarddecl_defaultparam.nim
    • macros.`==`(a, b: NimNode), see tests/lang_callable/macros/tmacros_various.nim
    • term-rewriting macros, see tests/lang_experimental/trmacros/trmacros_various2.nim
  • trees.exprStructuralEquivalentStrictSym and it's only usage in
    sem/semfoldnim have been removed

Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

A nice fix of a floating-point related edge case, thank you! The review comments I left are very minor and only concern code style and wording of some comments.

For the PR message, could you note down what the affected language features are (e.g., static parameters for generic types and procedures, term-rewriting macros, etc.; can go into the details section), for easier future assessment of what changed?

compiler/ast/trees.nim Outdated Show resolved Hide resolved
compiler/sem/semfold.nim Outdated Show resolved Hide resolved
tests/statictypes/tstatictypes.nim Show resolved Hide resolved
@zerbina zerbina added bug Something isn't working refactor Implementation refactor compiler/sem Related to semantic-analysis system of the compiler labels Feb 8, 2025
@Clyybber
Copy link
Contributor Author

Clyybber commented Feb 8, 2025

/merge

Thanks for the review!

Copy link

github-actions bot commented Feb 8, 2025

Merge requested by: @Clyybber

Contents after the first section break of the PR description has been removed and preserved below:


TODO

These could be included in this PR or be sepeate PRs:

  • Remove analysis for floats from sem/guards.nim sem: remove float analysis from sem/guards #1240
  • Ensure all float constants can round-trip as literals in the renderer
  • Ensure that the backend generate float constants bit equal
  • Issue a warning or hint when float equality doesn't match cmpFloatRep

@chore-runner chore-runner bot added this pull request to the merge queue Feb 8, 2025
Merged via the queue into nim-works:devel with commit f313cfc Feb 8, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler refactor Implementation refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants