Skip to content

test: Cover float comparison ops and WhileStmt float condition#239

Draft
Jaskirat-s7 wants to merge 2 commits intoarxlang:mainfrom
Jaskirat-s7:feature-issue-38-phase3
Draft

test: Cover float comparison ops and WhileStmt float condition#239
Jaskirat-s7 wants to merge 2 commits intoarxlang:mainfrom
Jaskirat-s7:feature-issue-38-phase3

Conversation

@Jaskirat-s7
Copy link
Copy Markdown
Contributor

PR Title

test(#38): cover float comparison operators and WhileStmt float condition


⚠️ Stacked PR — This branch builds on top of the Phase 2 PR (test/issue-38-uninit-var-fix) which is already under review. The Phase 2 changes (IfStmt float coverage + VariableDeclaration zero-init) are in that PR. Only the 2 commits below are new in Phase 3:

  • bf4c7aa — test_while_float_condition (WhileStmt float condition via fcmp_ordered)
  • e85c85f — Float comparison operators <=, >=, ==, != with Float32

PR Description

What this PR does

This PR adds 8 new tests across 2 test files that cover previously untested float-type code paths in LLVMLiteIRVisitor. No production code was changed — only test files.

Newly covered compiler paths

Test Operator Lines covered What it proves
test_binary_op_float_le Float32 <= 1115–1117 fcmp_ordered <= emitted correctly
test_binary_op_float_ge Float32 >= 1125–1127 fcmp_ordered >= emitted correctly
test_binary_op_float_eq Float32 == 1165–1168 fcmp_ordered == emitted correctly
test_binary_op_float_ne Float32 != 1188–1190 fcmp_ordered != emitted correctly
test_while_float_condition WhileStmt + Float32 1362–1363 Float loop condition cast to i1 correctly

All 5 of these code paths previously had 0% coverage.

Why the overall percentage looks unchanged

The 86% figure is a rounded integer over 1,793 tracked lines. Each additional covered line moves the raw percentage by ~0.056%. Covering 5 new lines moves it from 85.96%86.24% — both display as 86%. The raw missed line count dropped from 211 → 208 in llvmliteir.py.

To be transparent: a visible percentage jump to 87% requires covering ~18 lines in one PR. These tests cover 5 lines but specifically target previously untested float comparison semantics.

Why these tests matter (not just coverage)

The original issue (#38) was partly about tests that only verified the compiler didn't crash, not that it produced correct output. Every test in this PR:

  • Compiles the AST → LLVM IR → native binary via Clang
  • Executes the binary and asserts the printed string matches the expected result
  • Tests both branches (e.g., 2.0 <= 3.0 → "le_true" AND 3.0 <= 2.0 → "le_false")

For example, test_binary_op_float_le proves that 2.0 <= 3.0 produces "le_true" at runtime — not just that it compiles without crashing.

Test count

  • Before: 210 tests
  • After: 218 tests (all passing)

Files changed

  • tests/test_binary_op.py — +211 lines (4 new test functions, 8 parametrized cases)
  • tests/test_while.py — +54 lines (1 new test function)

@pytest.mark.parametrize(
"a_val, b_val, op, expected",
[
(2.0, 3.0, "<=", "le_true"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what is le here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

le is short for "less than or equal" (<=). le_true is printed when the condition holds (2.0 <= 3.0), and le_false
when it doesn't (3.0 <= 2.0). I am happy to rename to something clearer like "yes" / "no" or "lte_pass" / "lte_fail" if preferred.



@pytest.mark.parametrize("builder_class", [LLVMLiteIR])
def test_while_float_condition(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can parametrize it with the basic while functionality, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added Float32 and Float64 to test_while_expr. One issue came up during implementation was that the original increment used UnaryOp("++"), which generates an integer add instruction in LLVM IR. When floats were added, this produced add double which is invalid — floats need fadd.
It was fixed by replacing the increment with VariableAssignment + BinaryOp("+"), which correctly dispatches to fadd for floats and add for integers. All 6 types now pass (Int8/Int16/Int32/Int64/Float32/Float64).

module = builder.module()

cond = astx.BinaryOp(
op_code=">=",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i don't understand why are we hardcoding operator, op should be accepted as parameter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have merged all 4 functions (test_binary_op_float_le, test_binary_op_float_ge, test_binary_op_float_eq, test_binary_op_float_ne) into a single test_binary_op_float_comparison that accepts op_code as a parameter. The parametrize list now covers all 4 float comparison operators (<=, >=, ==, !=) with both true and false branches , 8 cases total, all are passing.



@pytest.mark.parametrize(
"a_val, b_val, expected",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@pytest.mark.parametrize(
"a_val, b_val, op, true_label, false_label",
[
(2.0, 3.0, "<=", "le_true", "le_false"),
(3.0, 2.0, ">=", "ge_true", "ge_false"),
(2.0, 2.0, "==", "eq_true", "eq_false"),
(1.0, 2.0, "!=", "ne_true", "ne_false"),
],
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i have updated to use your suggested structure exactly. 4 rows with a_val, b_val, op, true_label, false_label as separate parameters. true_label is used in the then block and asserted as output, false_label in the else
block. All 4 cases pass.

@Jaskirat-s7 Jaskirat-s7 force-pushed the feature-issue-38-phase3 branch 2 times, most recently from fe37ecd to 18f6ff6 Compare March 19, 2026 19:39
@Jaskirat-s7
Copy link
Copy Markdown
Contributor Author

@yuvimittal ,I have addressed all 4 review comments in the latest push:

1."what is le here?" — le/ ge/eq/ne are standard abbreviations for "less than or equal", "greater than or equal", etc. I have kept the labels as-is since they're consistent with operator naming conventions.
2."op should be accepted as parameter" — i have merged the 4 separate float comparison functions (float_le, float_ge, float_eq, float_ne) into a single test_binary_op_float_comparison accepting op as a parameter.
3.Suggested parametrize structure with true_label, false_label — i have updated to your exact suggestion: 4 rows with a_val, b_val, op, true_label, false_label as explicit parameters.
4."we can parametrize it with basic while functionality, right?" — i have added Float32/Float64 to test_while_expr. One issue: UnaryOp("++") generates add double (invalid for floats — needs fadd) , fixed by replacing the increment with VariableAssignment + BinaryOp("+"), which correctly dispatches fadd/add by type. All 6 types now pass (Int8/16/32/64 + Float32/64).

@Jaskirat-s7 Jaskirat-s7 force-pushed the feature-issue-38-phase3 branch from 18f6ff6 to 8c9278a Compare March 19, 2026 20:19
@Jaskirat-s7 Jaskirat-s7 requested a review from yuvimittal March 21, 2026 08:54
@xmnlab
Copy link
Copy Markdown
Contributor

xmnlab commented Mar 31, 2026

@Jaskirat-s7 could you rebase your branch on top of the upstream main please? I tried it here but it has some rebase conflicts to handle

@xmnlab xmnlab marked this pull request as draft March 31, 2026 21:46
@Jaskirat-s7 Jaskirat-s7 force-pushed the feature-issue-38-phase3 branch from 8c9278a to 6361bca Compare April 1, 2026 15:52
…in (rebased from PR arxlang#239)

- Replace LLVMLiteIR with Builder as LLVMBuilder (new public API after arxlang#272)
- Replace 'from irx.system import PrintExpr' with 'from irx.astx import PrintExpr'
- Extend test_while_expr parametrize to cover Float32 and Float64
- Change UnaryOp('++') update to VariableAssignment+BinaryOp('+') for float compat
- Add test_while_float_condition: Float32 condition covers fcmp_ordered path
- Add test_binary_op_float_comparison: parametrized <=, >=, ==, != with Float32
@Jaskirat-s7 Jaskirat-s7 force-pushed the feature-issue-38-phase3 branch from 6361bca to 16675d5 Compare April 2, 2026 20:18
@Jaskirat-s7
Copy link
Copy Markdown
Contributor Author

Jaskirat-s7 commented Apr 2, 2026

@xmnlab ,i have rebased onto main after #272 landed. The old branch had a chain of fixup commits that conflicted with main's already-updated test files, so I ported the meaningful end-state onto a fresh branch instead.

Changes to align with #272:

LLVMLiteIR → Builder as LLVMBuilder (new public API)
from irx.system import PrintExpr → from irx.astx import PrintExpr (PrintExpr moved to astx facade)
Loop update in test_while_expr changed from UnaryOp('++') to VariableAssignment + BinaryOp('+') — ++ only emits integer add, not fadd, causing invalid IR for float types
All 32 tests pass locally. Test coverage is identical to the original PR.

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