GH-49529: [R] CI job shows NOTE due to "non-API call" Rf_findVarInFrame#49530
Conversation
github-actions
Bot
added
awaiting committer review
| #define ARROW_R_DCHECK(EXPR) | ||
| #endif | ||
|
|
||
| #if (R_VERSION < R_Version(3, 5, 0)) |
There was a problem hiding this comment.
Removed this dead code as we're not supporting such old R versions any more
github-actions
Bot
added
awaiting changes
|
@github-actions crossbow submit test-r-linux-sanitizers |
|
Testing with the sanitizer job as it's the only place I've seen this pop up (I think it just has a newer version of R-devel) |
github-actions
Bot
added
awaiting change review
|
Revision: 2e1f1df Submitted crossbow builds: ursacomputing/crossbow @ actions-a98e5d04ad
|
|
I think this is good now - CI failure on that job is unrelated and due to some stringi weirdness. Before, we get the note: After, we don't: |
|
Do you we need (and maybe already have, I'm on mobile so haven't searched!) an issue for that stringi issue? |
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
This looks good if CI passes, one question (that might be ill-informed, I'm on my phone right now!)
| SEXP xp = Rf_findVarInFrame(self, arrow::r::symbols::xp); | ||
| if (xp == R_NilValue) { | ||
| #endif | ||
| if (xp == R_UnboundValue || xp == R_NilValue) { |
There was a problem hiding this comment.
Do you know why we need to add the xp == R_UnboundValue here? That's new right?
There was a problem hiding this comment.
My understanding is because the call to R_getVarEx() returns R_UnboundValue if it doesn't return anything for xp[1]
github-actions
Bot
added
awaiting merge
FYI, I have a PR up for this (#49581) though it's really an upstream issue |
…rInFrame (apache#49530) ### Rationale for this change CI job shows NOTE due to "non-API call" Rf_findVarInFrame ### What changes are included in this PR? Remove non-API calls to doesn't come up on CRAN notes ### Are these changes tested? I'll do some CI testing ### Are there any user-facing changes? No ### AI use Basically all of this, with Claude Opus 4.5 but I did ask multiple questions on the reasoning behind the changes and alternatives - don't understand 100% but looks reasonable to me * GitHub Issue: apache#49529 Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit ee7e210. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…rInFrame (apache#49530) ### Rationale for this change CI job shows NOTE due to "non-API call" Rf_findVarInFrame ### What changes are included in this PR? Remove non-API calls to doesn't come up on CRAN notes ### Are these changes tested? I'll do some CI testing ### Are there any user-facing changes? No ### AI use Basically all of this, with Claude Opus 4.5 but I did ask multiple questions on the reasoning behind the changes and alternatives - don't understand 100% but looks reasonable to me * GitHub Issue: apache#49529 Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>
…rInFrame (apache#49530) ### Rationale for this change CI job shows NOTE due to "non-API call" Rf_findVarInFrame ### What changes are included in this PR? Remove non-API calls to doesn't come up on CRAN notes ### Are these changes tested? I'll do some CI testing ### Are there any user-facing changes? No ### AI use Basically all of this, with Claude Opus 4.5 but I did ask multiple questions on the reasoning behind the changes and alternatives - don't understand 100% but looks reasonable to me * GitHub Issue: apache#49529 Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>
2 participants
Rationale for this change
CI job shows NOTE due to "non-API call" Rf_findVarInFrame
What changes are included in this PR?
Remove non-API calls to doesn't come up on CRAN notes
Are these changes tested?
I'll do some CI testing
Are there any user-facing changes?
No
AI use
Basically all of this, with Claude Opus 4.5 but I did ask multiple questions on the reasoning behind the changes and alternatives - don't understand 100% but looks reasonable to me