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

Remove conditionals from profiling #1302

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Remove conditionals from profiling #1302

merged 1 commit into from
Oct 17, 2024

Conversation

Izaakwltn
Copy link
Collaborator

There are still a few interior conditionals but nothing that affects the outward interface.

stylewarning
stylewarning requested changes Oct 11, 2024
View reviewed changes
library/system.lisp Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 2 times, most recently from 28f190e to bc3d762 Compare October 11, 2024 06:47
stylewarning
stylewarning requested changes Oct 11, 2024
View reviewed changes
library/system.lisp Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 4 times, most recently from 83c11f4 to 213f57f Compare October 14, 2024 17:01
stylewarning
stylewarning requested changes Oct 14, 2024
View reviewed changes
Copy link
Member

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

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

mostly nits and polish

library/system.lisp Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Show resolved Hide resolved
@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 2 times, most recently from 187aafb to 65aaee3 Compare October 14, 2024 17:42
@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 2 times, most recently from 140547d to 81beab0 Compare October 14, 2024 20:51
stylewarning
stylewarning requested changes Oct 14, 2024
View reviewed changes
Copy link
Member

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

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

some more bits and bobs

library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 2 times, most recently from 641d29b to d7e16e0 Compare October 15, 2024 00:06
@stylewarning
Copy link
Member

@Izaakwltn Do you have thoughts on returning a rational from the ->seconds call?

@Izaakwltn
Copy link
Collaborator Author

@Izaakwltn Do you have thoughts on returning a rational from the ->seconds call?

I didn't quite know what you meant but now it makes sense-

I think it would work well, though I don't believe there is currently a direct way to convert a fraction to various float types (besides Big-Float).

I can make an issue for an interface converting Fractions to inexact floats. Perhaps it should be a method in the Rational class .

@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 4 times, most recently from ecca322 to 9fad067 Compare October 15, 2024 17:37
@Izaakwltn
Copy link
Collaborator Author

@Izaakwltn Do you have thoughts on returning a rational from the ->seconds call?

I didn't quite know what you meant but now it makes sense-

I think it would work well, though I don't believe there is currently a direct way to convert a fraction to various float types (besides Big-Float).

I can make an issue for an interface converting Fractions to inexact floats. Perhaps it should be a method in the Rational class .

#1310

stylewarning
stylewarning requested changes Oct 16, 2024
View reviewed changes
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 2 times, most recently from abd26d6 to 9182fe4 Compare October 16, 2024 19:43
stylewarning
stylewarning requested changes Oct 16, 2024
View reviewed changes
Copy link
Member

@stylewarning stylewarning 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.

see comment about general/ or exact/

@Izaakwltn
Copy link
Collaborator Author

see comment about general/ or exact/

switched to exact/

stylewarning
stylewarning requested changes Oct 17, 2024
View reviewed changes
Copy link
Member

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

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

Last and final review, I promise. Just some naming and a few other nits.

library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Outdated Show resolved Hide resolved
library/system.lisp Show resolved Hide resolved
@Izaakwltn
Copy link
Collaborator Author

Changes changed

stylewarning
stylewarning requested changes Oct 17, 2024
View reviewed changes
library/system.lisp Outdated Show resolved Hide resolved
@Izaakwltn Izaakwltn force-pushed the profiling-fix branch 4 times, most recently from bdc1263 to af6f760 Compare October 17, 2024 22:51
@stylewarning stylewarning enabled auto-merge (rebase) October 17, 2024 22:52
@stylewarning stylewarning merged commit 52c1ac3 into main Oct 17, 2024
33 checks passed
@stylewarning stylewarning deleted the profiling-fix branch October 17, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stylewarning stylewarning stylewarning 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.
2 participants
@Izaakwltn @stylewarning