Skip to content

Support rep.int() in subset indexing#95

Merged
t-kalinowski merged 1 commit intomainfrom
rep-int-subscript
Feb 8, 2026
Merged

Support rep.int() in subset indexing#95
t-kalinowski merged 1 commit intomainfrom
rep-int-subscript

Conversation

@t-kalinowski
Copy link
Owner

Summary

  • Support rep.int() when used as an index inside x[...] (e.g. x[rep.int(1L, 5L)]) by lowering it to a Fortran integer vector constructor.
  • Scope support to subset indexing only; rep.int() used as a general value constructor errors to avoid silent type/semantic changes.
  • Fail cleanly for invalid indexing args (e.g. NULL) with user-facing errors (no internal slot-access failures).

Tests

  • Added tests/testthat/test-rep-int.R covering supported forms (positional, times=, non-literal times) and failure modes (outside indexing, NULL args).
  • R -q -e 'devtools::test()'

Add r2f handler for rep.int used as an index inside x[...], fail fast outside indexing, and add regression tests.
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: fe33d9b357

ℹ️ 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 +95
len_expr <- r2size(times_arg, scope)
if (is.null(len_expr) || is_scalar_na(len_expr)) {
len_expr <- NA_integer_

Choose a reason for hiding this comment

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

P1 Badge Use times value for rep.int length, not its size

The length you derive for the replicated index uses r2size(times_arg), which returns the size of the times expression (usually 1 for a scalar), not its runtime value. In x[rep.int(2L, n)] where n is a scalar integer (e.g., 4), this sets the index vector dims to length 1, so the subscript is treated as scalar during drop/shape inference in [ and the output shape becomes incorrect. The generated Fortran array constructor will still produce length n, so the wrapper’s metadata can disagree with the actual result. Consider propagating the actual times expression (or an unknown length) instead of r2size(times_arg).

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 (1de5107) to head (fe33d9b).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #95   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files          27       27           
  Lines        5754     5754           
=======================================
  Hits         5339     5339           
  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
Copy link
Owner Author

t-kalinowski commented Feb 8, 2026

Re: len_expr <- r2size(times_arg, scope)

Here r2size() is used as a dims-expression helper: for scalar integer times it resolves to the runtime value expression (e.g. for x[rep.int(2L, n)] with n an integer scalar arg, r2size(n, scope) is n), not size(times).

This is exercised by tests/testthat/test-rep-int.R (the "rep.int supports non-literal times in subset indexing" test). If the handler were inferring length 1, [ would treat the index as scalar-like and the quick result would not match R (length would be wrong), so the test would fail.

@t-kalinowski t-kalinowski merged commit 8b5de6d into main Feb 8, 2026
14 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