Skip to content

Fix subscript hoisting; correct coercion/matrix lowering#94

Merged
t-kalinowski merged 5 commits intomainfrom
fix/subscript-hoist
Feb 8, 2026
Merged

Fix subscript hoisting; correct coercion/matrix lowering#94
t-kalinowski merged 5 commits intomainfrom
fix/subscript-hoist

Conversation

@t-kalinowski
Copy link
Owner

This PR fixes several lowering/translation issues hit by simplified lowering patterns:

  • Fix [ ... ] so index expressions are compiled with the caller hoist context, avoiding invalid inline block ... end block expressions inside Fortran array designators (e.g. rev(seq_len(n)) in a multi-d subscript).
  • Make as.double(<array>) and as.integer(<array>) drop dimensions (return a vector) by reshaping rank>1 results, matching base R semantics and fixing slice assignments like out[1:6] <- as.double(a).
  • Fix matrix(<non-scalar expr>, nrow=, ncol=) lowering by emitting an explicit reshape(...) so the generated Fortran ranks match.
  • Improve the bad rank error for rank-0 scalar declarations to point users at integer(1) / double(1).

Regression tests added:

  • tests/testthat/test-subscript-hoisted-temporaries.R
  • tests/testthat/test-as-double.R
  • tests/testthat/test-matrix.R
  • tests/testthat/test-c-wrapper-public-api-coverage.R

Tested: devtools::test()

Copy link

@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: 02082b16b1

ℹ️ 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 93 to 97
source <- glue("{src}")
Fortran(
glue(
"reshape({source}, [{bind_dim_int(rows)}, {bind_dim_int(cols)}], pad = {source})"
),

Choose a reason for hiding this comment

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

P2 Badge Avoid double-evaluating matrix() data expression

The new matrix() lowering uses reshape(..., pad = {source}), which inserts the data expression twice. In Fortran, all actual arguments are evaluated before the intrinsic call, so a non-scalar data expression (e.g., runif(4) or any user-defined function with side effects) will be evaluated twice. That changes semantics versus R, which evaluates data once, and can produce different values or duplicated side effects. Consider hoisting data into a temporary and using that name in both reshape and pad, or only supplying pad when it’s needed and the source is already a plain symbol.

Useful? React with 👍 / 👎.

@codecov
Copy link

codecov bot commented Feb 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.78%. Comparing base (63d755e) to head (9d1ced5).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #94   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files          27       27           
  Lines        5749     5754    +5     
=======================================
+ Hits         5334     5339    +5     
  Misses        415      415           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@t-kalinowski t-kalinowski merged commit 1de5107 into main Feb 8, 2026
8 checks passed
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.

1 participant