Skip to content

Git issue 040#41

Merged
ckormanyos merged 11 commits intomodm-io:masterfrom
ckormanyos:git_issue_040
May 24, 2025
Merged

Git issue 040#41
ckormanyos merged 11 commits intomodm-io:masterfrom
ckormanyos:git_issue_040

Conversation

@ckormanyos
Copy link
Copy Markdown
Collaborator

@ckormanyos ckormanyos commented May 12, 2025

This PR is intended to repair missing C++11 ` functions.

It addresses #40.

In my naive analysis, simply the position of a preprocessor #if-#else-#endif was actually removing C++11 math functions from the header.

@ckormanyos ckormanyos mentioned this pull request May 12, 2025
Closed
@ckormanyos
Copy link
Copy Markdown
Collaborator Author

ckormanyos commented May 12, 2025

Hi @salkinium and @chris-durand this fixes the fpclassify omission (and also a few other C++11 mathematical functions).

@ckormanyos
Copy link
Copy Markdown
Collaborator Author

ckormanyos commented May 12, 2025 •
edited
Loading

When using the avr-gcc image in CI to build the examples, I had breifly activated 64-bit double and, indeed, that linker is broken. So I walked the example back to 32-bit double.

This might be related to #38, as there is/was definitely a problem that I simply swept under the rug here.

@salkinium salkinium linked an issue May 14, 2025 that may be closed by this pull request
<cmath> does not publish std::fpclassify() #40
Closed
salkinium
salkinium approved these changes May 14, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Sure!

jwakely
jwakely reviewed May 14, 2025
View reviewed changes
Comment thread include/cmath
@ckormanyos ckormanyos marked this pull request as draft May 14, 2025 19:43
@ckormanyos
Copy link
Copy Markdown
Collaborator Author

ckormanyos commented May 14, 2025

Hi @salkinium and @jwakely based on the review comments, I have converted this into a draft PR until I/we straighten out the math declarations more clealry.

Cc: @chris-durand

@ckormanyos ckormanyos mentioned this pull request May 15, 2025
Closed
jwakely
jwakely reviewed May 15, 2025
View reviewed changes
Comment thread include/bits/stl_iterator.h
jwakely
jwakely reviewed May 15, 2025
View reviewed changes
Comment thread include/bits/stl_iterator.h Outdated
@ckormanyos
Copy link
Copy Markdown
Collaborator Author

ckormanyos commented May 15, 2025

OK folks, the latest, greatest <cmath> and improved docs is ready to go now. I've handled all of the review comments from @jwakely (thank you for these). We have closed #42 and will fix <cmath> here in this one.

So @chris-durand and @salkinium in I believe this one is ready now.

@ckormanyos ckormanyos marked this pull request as ready for review May 15, 2025 11:25
@ckormanyos
Copy link
Copy Markdown
Collaborator Author

ckormanyos commented May 15, 2025

This PR fixes #40

jwakely
jwakely approved these changes May 15, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

LGTM

@ckormanyos
Copy link
Copy Markdown
Collaborator Author

ckormanyos commented May 22, 2025

Hi @salkinium and @chris-durand.

I think this one is good to go. Also, I could probably take a more active role in the support of this thing. So if you'd like to have that discussion, I'm all ears.

There is a lot of value in this work. We could discuss the future evolution of this work any time.

Cc: @jwakely

@salkinium
Copy link
Copy Markdown
Member

salkinium commented May 22, 2025

Also, I could probably take a more active role in the support of this thing.

That would be great! You've already polished this repo a lot, so I've upgraded you to write access, so you should be able to merge this PR now.

@ckormanyos
Copy link
Copy Markdown
Collaborator Author

ckormanyos commented May 24, 2025

I could probably take a more active role in the support of this

That would be great! You've already polished this repo a lot, so I've upgraded you to write access

Thanks Niklas (@salkinium) I'll do my best

Cc: @chris-durand

@ckormanyos ckormanyos merged commit 5354296 into modm-io:master May 24, 2025
2 checks passed
@ckormanyos ckormanyos deleted the git_issue_040 branch May 24, 2025 06:46
@jwakely jwakely mentioned this pull request Sep 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@salkinium salkinium salkinium approved these changes

+1 more reviewer

@jwakely jwakely jwakely approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<cmath> does not publish std::fpclassify()

3 participants

@ckormanyos @salkinium @jwakely