Skip to content
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

Allow staged installation + import Rcpp::sourceCpp() + fix error in ConvertHexToBytes #7

Closed
wants to merge 13 commits into from

Conversation

elipousson
Copy link
Contributor

I'm not sure if I have this all done right but I think this pull request may close out #4 and potentially could help with #6. Here are the key changes:

  • Added Rcpp::sourceCpp() to imports (following guidance from Advanced R on using Rcpp in a package)
  • Restored default setting for staged installation
  • Corrected what appears to be a potential typo in checking for odd-length vectors (swapping | for == in utilities.cpp)

Building the package still returns a variety of warnings about the C++ code. I was able to address a couple obvious issues (removing unused variables this_compound_index and MAXPAGE) but I don't really know enough C++ to fix the other issues.

I also added a NEWS.md file to keep track of changes.

@AllanCameron
Copy link
Owner

Hi @elipousson. I have fixed the C++ code so compilation should go ahead OK. The checks seem to work out, except I had to reinstate the no-staged compilation as this was causing an error. The ConvertHexToBytes function is correct - I know it doesn't look it, but it uses a bitwise OR to check whether the vector length is odd or even. Using == is wrong here. I think the staged installation problem has to do with the way the pdf paths are stored. I'll have a look at this and try to fix it.

@elipousson
Copy link
Contributor Author

Glad you had a chance to get back to this, thank you! I'll close this PR since you already incorporated the changes. Looking forward to using the package more in the future.

@elipousson elipousson closed this May 5, 2023
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.

2 participants