Skip to content

Conversation

@jonkeane
Copy link
Member

@jonkeane jonkeane commented Jul 7, 2024

Rationale for this change

CRAN

What changes are included in this PR?

Remove so-called non-api calls

Are these changes tested?

With tests

Are there any user-facing changes?

Hopefully not

This PR contains a "Critical Fix".

@github-actions
Copy link

github-actions bot commented Jul 7, 2024

⚠️ GitHub issue #43044 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added Component: R awaiting committer review Awaiting committer review labels Jul 7, 2024
Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

I think this is ok, just one suggestion for something else to remove. AFAICT we only call r_string_size on a STRING_ELT from a STRSXP, and similarly for utf8_string/utf8_strings. Rf_translateCharUTF8 should always work on those inputs.

Here's the R internals version of IS_ASCII etc.; LEVELS is defined as sxpinfo.gp, an opaque bitmask. Even if this change turns out not to be correct, I don't think we want to be touching sxpinfo.gp for anything ourselves. https://github.com/wch/r-source/blob/0ebd0b0b574847597ac19fbf0bbbf31e3197e84b/src/include/Defn.h#L832-L853

Comment on lines 44 to 45
#define UTF8_MASK (1 << 3)
#define ASCII_MASK (1 << 6)
Copy link
Member

Choose a reason for hiding this comment

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

I think you can delete these too, they were only used by the macros you removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right of course

@nealrichardson
Copy link
Member

I think this is ok, just one suggestion for something else to remove. AFAICT we only call r_string_size on a STRING_ELT from a STRSXP, and similarly for utf8_string/utf8_strings. Rf_translateCharUTF8 should always work on those inputs.

Here's the R internals version of IS_ASCII etc.; LEVELS is defined as sxpinfo.gp, an opaque bitmask. Even if this change turns out not to be correct, I don't think we want to be touching sxpinfo.gp for anything ourselves. https://github.com/wch/r-source/blob/0ebd0b0b574847597ac19fbf0bbbf31e3197e84b/src/include/Defn.h#L832-L853

Ah great, you've found the line in the scriptures to confirm that #43044 (comment)

@jonkeane
Copy link
Member Author

jonkeane commented Jul 7, 2024

Ah great, you've found the line in the scriptures to confirm that #43044 (comment)

It took many nights of studying the holy text

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 7, 2024
@jonkeane jonkeane merged commit ca49843 into apache:main Jul 7, 2024
@jonkeane jonkeane removed the awaiting changes Awaiting changes label Jul 7, 2024
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit ca49843.

There were 5 benchmark results indicating a performance regression:

The full Conbench report has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.

jonkeane added a commit that referenced this pull request Jul 18, 2024
### Rationale for this change

CRAN 

### What changes are included in this PR?

Remove so-called non-api calls

### Are these changes tested?

With tests

### Are there any user-facing changes?

Hopefully not

**This PR contains a "Critical Fix".**
* GitHub Issue: #43044

Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants