New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use 'Rf_mkCharLenCE() as appropriate #917

Merged
merged 2 commits into from Nov 3, 2018

Conversation

Projects
None yet
2 participants
@kevinushey
Contributor

kevinushey commented Oct 25, 2018

Closes #916, taking option (1) -- signal an error if an embedded NUL byte is encountered, as R does.

@@ -395,9 +395,12 @@ namespace Rcpp {
enc = encoding;
if (valid) {
data = Rcpp_ReplaceObject(data, Rf_mkCharCE(Rf_translateCharUTF8(data), encoding));
const void* vmax = vmaxget();

This comment has been minimized.

@kevinushey

kevinushey Oct 25, 2018

Contributor

I noticed that translateCharUTF8() has, in the R sources:

https://github.com/wch/r-source/blob/61132afa07832f4e855da35773fa1341f1a8b481/src/main/sysutils.c#L992-L994

so it's recommended that we manage the stack after usage. Not sure if we should do this in this PR or just leave it out for now though.

@eddelbuettel eddelbuettel referenced this pull request Oct 26, 2018

Merged

Fix/encoding #30

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Oct 26, 2018

It just occurred to me that we don't want the R error (ie the longjmp) so we'll have to do our own error checking before calling Rf_mkCharLenCE().

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Oct 26, 2018

That sounds like a worthwhile change.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Oct 27, 2018

@kevinushey Do you want to add an error check before Rf_mkCharLenCE() ?

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Oct 27, 2018

Yes, sorry -- I'll try to get to it soon.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Oct 27, 2018

No worries -- that was the right call. No rush.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Nov 2, 2018

I'm confused. Did you amend the initial commit in this PR? I am only seeing one commit when there should have been two.

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Nov 2, 2018

Yes, I amended the original commit -- I tend to do this on feature branches just to keep the commit history clean. (Although perhaps it's better to just squash and merge rather than retroactively change history on the branch)

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Nov 2, 2018

Yes, we could settle on just squashing. I also don't single / non-parallel branches merging back.

In any event here the downside was that your repo co-user, ie me, got lost :).

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Nov 2, 2018

Sorry for the confusion :-)

Ultimately, this should be ready to go now.

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Nov 2, 2018

It looks like the h5 package is deprecated, so updated to CRAN may not be as timely as we might like. For that reason I'm going to just work around the issue and use the old-school Rf_mkCharCE() when h5 is detected.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Nov 2, 2018

Interesting. Have we ever done that? Explicitly accomodate other packages?

(And yes, hdf5 package seem to be a in constant state of flux. Too bad CRAN doesn't just patch...)

Edit: I also know the h5 maintainer so I can always ask...

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented Nov 2, 2018

Interesting. Have we ever done that? Explicitly accomodate other packages?

Not that I can recall, but given that it's one package with a simple workaround, I don't mind having this live in Rcpp if it means existing code depending on h5 can still work fine. What do you think?

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Nov 2, 2018

It is on the hack-ish side of things but if we mean it to be temporary I can live with it. Let me email Mario and see what he says.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Nov 2, 2018

Oh, and I just now see the rather nice ticket you opened there already. Well done.

@eddelbuettel eddelbuettel merged commit 2f72ff8 into master Nov 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment