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

fix(fmath.h): One more fast_exp fix #4275

Merged
merged 1 commit into from
May 26, 2024

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented May 25, 2024

In PR #4268, I accidentally changed the clause addressing what is seemingly a bug with older MSVS. I'm just putting things back the way they were. It's not a new change, just restoring this line closer to the way it was about 3 commits back.

In PR 4268, I accidentally changed the clause about things seeming
buggy with older MSVS. I'm just putting things back the way they were.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Copy link
Collaborator

@ThiagoIze ThiagoIze left a comment

Choose a reason for hiding this comment

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

Shoot, when I saw this removed I was hoping this was because it was fixed on older msvc on account of us no longer relying on undefined behavior. Oh well, if we aren't sure it's fixed, let's play it safe.

@lgritz
Copy link
Collaborator Author

lgritz commented May 26, 2024

I think I was hoping that my last fix took care of this, too, but apparently not, and it's a different symptom than what got fixed with the undefined behvaior fix. It's hard to reproduce, and only on Windows while using the older MSVS (whereas the other easily reproduced in gcc as well), so I think this one really is a compiler bug, but I admit that my confidence in that claim is lower than it was before, only because the other thing did also affect fast_exp. But I can't find any identifiable problems, so the only plan I have at the moment is disabling this one thing on the one older compiler that is problematic, unless/until it reproduces anywhere else.

@lgritz lgritz merged commit becf0f6 into AcademySoftwareFoundation:master May 26, 2024
24 checks passed
@lgritz lgritz deleted the lg-exp branch May 26, 2024 20:47
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 this pull request may close these issues.

2 participants