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.

Sign up for GitHub

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

Jump to bottom

fix math.frexp function signature #16725

Merged
merged 19 commits into from
Feb 17, 2021
Merged

fix math.frexp function signature #16725

merged 19 commits into from
Feb 17, 2021

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Jan 15, 2021 •
edited
Loading

  • unexport c functions
  • fix generics function signature
  • fix wrong importc function

@ringabout ringabout marked this pull request as draft January 15, 2021 07:14
ringabout
ringabout commented Jan 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
@ringabout ringabout marked this pull request as ready for review January 15, 2021 07:57
timotheecour
timotheecour reviewed Jan 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
timotheecour
timotheecour reviewed Jan 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
timotheecour
timotheecour reviewed Jan 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
timotheecour
timotheecour reviewed Jan 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
@ringabout ringabout marked this pull request as draft January 16, 2021 11:39
@ringabout ringabout marked this pull request as ready for review February 1, 2021 14:52
timotheecour
timotheecour reviewed Feb 2, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
timotheecour
timotheecour approved these changes Feb 2, 2021
View reviewed changes
Copy link
Member

@timotheecour timotheecour left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM; remaining comments can be addressed in future PR; in particular:

@timotheecour timotheecour mentioned this pull request Feb 12, 2021
Closed
3 tasks
@ringabout ringabout requested a review from Araq February 12, 2021 02:57
@ringabout
Copy link
Member Author

ringabout commented Feb 13, 2021 •
edited
Loading

@Araq
THIS PR corrects the terrible function signature before, please review this PR.

@ringabout ringabout changed the title better frexp fix math.frexp function signature Feb 13, 2021
konsumlamm
konsumlamm suggested changes Feb 14, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
lib/pure/math.nim Outdated Show resolved Hide resolved
lib/pure/math.nim Outdated Show resolved Hide resolved
konsumlamm
konsumlamm reviewed Feb 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
Araq
Araq reviewed Feb 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour mentioned this pull request Feb 15, 2021
Merged
timotheecour
timotheecour reviewed Feb 15, 2021
View reviewed changes
lib/pure/math.nim Outdated Show resolved Hide resolved
timotheecour
timotheecour approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

LGTM but we should followup soon with a PR that makes signbit preserved for +-0 true

@Araq Araq merged commit 35e1499 into nim-lang:devel Feb 17, 2021
@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Feb 21, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@ringabout @ardek66
fix math.frexp function signature (nim-lang#16725)
1c4093d
@konsumlamm
Copy link
Contributor

Shouldn't the new overload have a since annotation? Or does this count as fixing an existing function?

@timotheecour
Copy link
Member

#17048

...

  • if an overload is added

@konsumlamm konsumlamm mentioned this pull request Mar 31, 2021
Merged
1 task
@ringabout ringabout removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jun 29, 2021
@ringabout
Copy link
Member Author

Remove TODO label, this PR is succeeded by #17588

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konsumlamm konsumlamm konsumlamm requested changes

@Araq Araq Araq left review comments

@timotheecour timotheecour timotheecour approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ringabout @konsumlamm @timotheecour @Araq