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

Use the standard library fabs() #1495

Closed
darbyjohnston opened this issue Dec 7, 2022 · 6 comments
Closed

Use the standard library fabs() #1495

darbyjohnston opened this issue Dec 7, 2022 · 6 comments

Comments

@darbyjohnston
Copy link
Contributor

I noticed there is a custom implementation of fabs() in rationalTime.h, it might be nice to replace it with the function from the standard library:
https://en.cppreference.com/w/cpp/numeric/math/fabs

It would reduce the amount of code in OpenTime and remove a function that should probably not be part of the API.

@darbyjohnston
Copy link
Contributor Author

Oops, now I remember why there is a need for the custom implementation. It uses constexpr which is not available from the standard library until C++23. I'll create a separate PR to add a comment about this.

@meshula
Copy link
Collaborator

meshula commented Dec 7, 2022

@darbyjohnston It wasn't just constexpr, although that is the comment on the commit. We wanted specific behavior around nan and inf.

To quote the C spec

An implementation may give zero and non-numeric values (such as infinities and NaNs) a sign or may leave them unsigned. Wherever such values are unsigned, any requirement in this International Standard to retrieve the sign shall produce an unspecified sign, and any requirement to set the sign shall be ignored.

There was a corner case sadly not captured in a regression test, where signs were not propagating as expected. Maybe someone else recalls more specific details. As I recall, this bit masking implementation was a deliberate choice in order to avoid underspecified behavior in the C spec.

@darbyjohnston
Copy link
Contributor Author

Interesting, we should probably add a comment that explains this. I had forgotten and my first instinct seeing it again was that it was possibly duplicated code. Also should it be a part of the public API?

@meshula
Copy link
Collaborator

meshula commented Dec 7, 2022

It's only in the public API because of the constexpr business. It could be made private copied twice within RationalTime and TimeRange. I'd have no argument with that, as I do concur with your implication that it is an implementation detail.

It's only in the public API
Because of the constexpr business
It could be made private
Copied twice within RationalTime and TimeRange

I'd have no argument with that
As I do concur with your implication
That it is an implementation detail
Best left hidden from the user's view

But still it remains in the API
A small cog in the machine
Doing its job without complaint
A faithful servant to the code it serves

Though its purpose may not be clear
To those who use the API without fear
It plays its role without protest
A silent partner in the quest

For efficient, reliable software
That does its job without a fuss
So let us give thanks to the constexpr business
And the small part it plays in our code's success.

  • GPT3

@darbyjohnston
Copy link
Contributor Author

It would be nice to hide but I'm not a fan of duplicating the code even though it's a fairly small function. Another way of hiding it would be as a protected function in a base class but that seems a bit convoluted. I think just a comment explaining it's use would be good, I'll make a separate PR for that.

I wonder if GPT3 could write Doxygen for us. :)

@meshula
Copy link
Collaborator

meshula commented Dec 7, 2022

I think GPT3 could in fact write documentation for us, or to put it a different way, it could write a rather prosey explanation that might provide a good starting point for clearly explaining what some code does. You can grab a function and ask it "Explain the following code: ". Josh has been experimenting with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants