Skip to content

test(softfloat): add IEEE 754 edge cases for min/max functions#96

Open
euinharry wants to merge 2 commits intogevico:mainfrom
euinharry:test/minmax-ieee754
Open

test(softfloat): add IEEE 754 edge cases for min/max functions#96
euinharry wants to merge 2 commits intogevico:mainfrom
euinharry:test/minmax-ieee754

Conversation

@euinharry
Copy link
Copy Markdown

Add test cases to validate softfloat min/max operations against IEEE 754-2008.
The new tests cover the missing scenarios mentioned in issue #61:

  • Signed zero comparison behavior
  • Quiet NaN interaction with finite values
  • Signaling NaN exception triggering
  • Signed number ordering validation
    These cases ensure the operations adhere to the standard's requirements.

Fix: #61
Signed-off-by: euinharry 3411893753@qq.com

@Lfan-ke
Copy link
Copy Markdown
Collaborator

Lfan-ke commented Apr 25, 2026

@codex Please help review this PR.

@Lfan-ke
Copy link
Copy Markdown
Collaborator

Lfan-ke commented Apr 25, 2026

Code Standards Review

PR: test(softfloat): add IEEE 754 edge cases for min/max functions (#96)
Commits reviewed: 2
Reviewed at: 2026-04-25 08:05 UTC

The following issues were found ⚠️

  • a9bc3ac tests(softfloat): add min/max IEEE 754 edge cases for # — headline does not follow Conventional Commits format <type>(<scope>): <subject>. Valid types: feat, fix, refactor, docs, ci, test, build, chore.
  • dd8a53e tests(softfloat): add min/max IEEE 754 edge cases for # — missing Signed-off-by:. Fix with git commit --amend -s (or git rebase --exec 'git commit --amend --no-edit -s').
  • dd8a53e tests(softfloat): add min/max IEEE 754 edge cases for # — headline does not follow Conventional Commits format <type>(<scope>): <subject>. Valid types: feat, fix, refactor, docs, ci, test, build, chore.

RustSBI contribution requirements:

  1. Every commit must have Signed-off-by: — use git commit -s or git commit --amend -s.
  2. Commit headlines must follow Conventional Commits: <type>(<scope>): <subject>.
  3. All commit messages must be written in English.
  4. Headline ≤ 72 characters; body lines ≤ 100 characters.
  5. Use imperative present tense (add, not added).

Please amend the offending commits and force-push before requesting merge.


Automated review by PR Monitor.

Signed-off-by: euinharry <3411893753@qq.com>

tests(softfloat): add min/max IEEE 754 edge cases for gevico#61
@euinharry euinharry force-pushed the test/minmax-ieee754 branch from dd8a53e to 6ec4f3f Compare April 25, 2026 08:15
@zevorn
Copy link
Copy Markdown
Member

zevorn commented Apr 25, 2026

@codex Please review this PR

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ec4f3f288

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +261 to +263
let mx = snan.max(finite, &mut e);
assert_eq!(mx.to_f32(), 3.14);
assert!(e.flags().contains(ExcFlags::INVALID));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clear flags before asserting max(sNaN, finite) behavior

FloatEnv exception flags are sticky, so after snan.min(finite, &mut e) sets INVALID, the later assert!(e.flags().contains(ExcFlags::INVALID)) will pass even if max stops raising INVALID. In this test, that makes the max check non-independent and can hide regressions in the max path; reset flags (or use a fresh env) before the max call so the assertion verifies the second operation itself.

Useful? React with 👍 / 👎.

@Lfan-ke
Copy link
Copy Markdown
Collaborator

Lfan-ke commented Apr 25, 2026

Code Standards Review

Commit 6ec4f3f: the type tests is not a valid Conventional Commits type. The correct singular form is test:

# current (wrong):
tests(softfloat): add min/max IEEE 754 edge cases for #61

# correct:
test(softfloat): add min/max IEEE 754 edge cases for #61

Fix:

git commit --amend -m "test(softfloat): add min/max IEEE 754 edge cases for #61" -s
git push --force-with-lease

Reviewed against machina contribution guidelines.

@zevorn
Copy link
Copy Markdown
Member

zevorn commented Apr 25, 2026

please fix:

Run make fmt-check
cargo fmt --all --check
Diff in /home/runner/work/machina/machina/tests/src/softfloat/edge_cases.rs:282:
     // max(-1.0, +1.0) -> +1.0
     assert_eq!(d.max(c, &mut e).to_f32(), 1.0);
 }
-
 
make: *** [Makefile:34: fmt-check] Error 1
Error: Process completed with exit code 2.

Modify monitor.rs and service.rs in tests/src to complete test logic
and verify module functionality as required.

Signed-off-by: euinharry <3411893753@qq.com>

Signed-off-by: euinharry <3411893753@qq.com>
@Lfan-ke
Copy link
Copy Markdown
Collaborator

Lfan-ke commented Apr 25, 2026

Code Standards Review

New commit 018be2f (test(tests): add and update monitor & service test cases) passes all standards — LGTM ✅.

Note: the existing 6ec4f3f commit still uses tests instead of test (see prior review). No further action needed on 018be2f itself.

Reviewed against machina contribution guidelines.

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.

[Open Source Workshop] Add IEEE 754 edge cases for SoftFloat min/max

3 participants