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 optional instead of infinity #1840

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use optional instead of infinity #1840

wants to merge 2 commits into from

Conversation

levinli303
Copy link
Collaborator

No description provided.

@levinli303 levinli303 marked this pull request as draft August 8, 2023 11:26
@levinli303 levinli303 force-pushed the optional branch 3 times, most recently from ca48642 to d9a17f2 Compare August 8, 2023 11:45
@levinli303 levinli303 requested a review from a team August 8, 2023 11:45
@levinli303 levinli303 marked this pull request as ready for review August 8, 2023 11:45
@levinli303 levinli303 force-pushed the optional branch 2 times, most recently from 7f93aa3 to d8f003b Compare August 8, 2023 11:53
tdb = phase->startTime();
else if (tdb > phase->endTime())
tdb = phase->endTime();
tdb = phase->clamp(tdb);
Copy link
Collaborator

Choose a reason for hiding this comment

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

clamp to [0, 1]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is to clamp between start time and end time of a phase.

@375gnu
Copy link
Collaborator

375gnu commented Aug 8, 2023

what's wrong with infinity?

@sonarcloud
Copy link

sonarcloud bot commented Aug 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
1.6% 1.6% Duplication

warning The version of Java (11.0.17) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@levinli303
Copy link
Collaborator Author

levinli303 commented Aug 8, 2023

what's wrong with infinity?

i was hoping to enable fastmath sometime. frustum.cpp checks isinf which can easily be avoided. i have isinf checks for startTime/endTime of phases in other frontends id rather not replace with checking against 1e9

@ajtribick
Copy link
Collaborator

I have mixed feelings about -ffast-math - some bits of it make a lot more sense than others and I would prefer to rely on the individual sub-flags. Of the ones that aren't switched on by default in C++ on x86_64 (-fno-signaling-nans, -fno-rounding-math, -fexcess-precision=fast), I think -fno-math-errno and -fno-trapping-math are ones we should probably be using: errno for math functions makes sense from the point of view of the C language, which targets machines where floating point representations do not have infinities/NaNs, but is fairly redundant in the case of IEEE floats. -freciprocal-math and -fassociative-math also seem pretty reasonable, in particular the latter seems to be useful for vectorization.

The -ffinite-math-only flag seems much less useful to the point of being potentially dangerous - it makes detecting these cases in user-provided binary-format input files harder (if you've told the system that only finite numbers are present, the compiler is allowed to optimize away checks for these values), and combined with disabling errno makes it really hard to detect the results of calculations doing overflows. -fno-signed-zeros is perhaps less clear-cut, although enabling it prevents roundtripping of certain CMOD files (including the iss.cmod used in the integration tests) from binary to text to binary again.

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.

3 participants