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

Begin migration of calc functions to methods #3489

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Feb 25, 2024

This will supersede functions such as calc.pow(5, 6) by 5.pow(6) or int.pow(5, 6), for example.
The calc functions will remain available for the time being, however.

Implementation details

Some things still have to be sorted out, but here are the main highlights (currently):

  • Right now, I'm moving most functions to methods under i64 and f64.
  • There is currently quite a bit of duplication with calc; I'm trying to trim it down as much as possible, but there's also this idea of trying not to depend so much on calc in order to eventually be able to remove as much as possible from it in the future.
  • I'm not sure if I should add .abs() methods for length, ratio and fr as well (currently, I've added it to i64, f64 and angle). But it sounds plausible at first, with length being the only possible problem, given it already has an abs field and a to_absolute method.
  • calc.min and calc.max are in a bit of a weird state right now. Those methods accept any comparable values, so it's hard to tell what I should do with it; perhaps we should add methods for each comparable value, but that sounds a bit excessive in principle. For now, I've added int.min and float.min which accept anything that compares to numbers.
  • The remaining methods not mentioned above were pretty smoothly dealt with.

Worth noting that I didn't add any warnings to migrated methods; tell me if that'd be desirable.

Docs

I did not update the docs, only adapted existing docs to the new methods. Some more major docs changes (involving a Ctrl+F search of calc across all docstrings) are in order though, but I'm not sure how much of it I should do in this PR instead of separately, if at all.

Regarding the #[scope] macro

Constants

I wanted to move calc.e, calc.nan etc. to float.e, float.nan etc., but currently the f64Ext trait generated by #[scope] does not appear to add the constants correctly.

Functions

Of note, I had to append _ to some method names (e.g. pow_) because the #[scope] proc macro (or perhaps #[func]) kept generating some code which tried to use i64::pow (or similar) directly, instead of i64Ext::pow, thus leading to an error of "unexpected parameters" when the signatures didn't match. I didn't have to add an underscore when the function signatures did match. Note that the underscore isn't visible to the end user (int.pow works). This might be interesting to somehow fix in the future (though feels out of scope for this PR).

As a result, i64Ext and f64Ext were generating methods called pow__data (from the {ident}_data format), triggering a non_snake_case warning. Since these methods are internal, I've worked around it by simply ignoring the warning for the generated trait, but it's worth mentioning.

I also added pub(super) to the generated i64Ext and f64Ext traits so their implementations can be reused by each other.

Finally, the lack of self: Spanned<Self> led me to use the callsite span for errors related to self on multiple occasions. I think that works for now. (I guess we'll be able to more trivially solve that when Rust implements support for arbitrary receivers.)

@laurmaedje laurmaedje added the waiting-on-design This PR or issue is blocked by design work. label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-design This PR or issue is blocked by design work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants