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

Include array header to appease g++-12 #57

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

eddelbuettel
Copy link
Contributor

@eddelbuettel eddelbuettel commented Dec 16, 2022

While testing an upgrade of package BH against all its reverse-dependencies (see this issue for details) I noticed that rlas failed to build.

The cause is unrelated to the BH upgrade and due to the fact that I ran the tests with g++-12 (on Debian testing). The newer compiler wants an explicit #include for the std::array data structure. Once that one line is added the package installs and tests fine under g++-12.

The change is very simple. It would be lovely if you could update the package at CRAN even though this is more of a g++ issue than a BH issue.

@Jean-Romain Jean-Romain merged commit 152bff5 into r-lidar:master Dec 16, 2022
Jean-Romain added a commit that referenced this pull request Dec 16, 2022
@Jean-Romain
Copy link
Collaborator

Thanks. I need to fix numerous new errors that spontaneously appeared on CRAN before to update rlas. I will do that on Monday hopefully.

@eddelbuettel
Copy link
Contributor Author

Yes, the sprintf -> snprintf is annoying. I have done it in two packages.

Thanks for the swift merge though!

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Dec 19, 2022

Hi @eddelbuettel ,

Since we started the discussion about sprintf I'm wondering if you could kindly give me some tips before I ask on Stackoverflow? First I've seen nothing about sprintf in Writing R extensions. Second I have 784 occurences from the third party library included. I tried a naive automatic replacement with grep and sed but of course the modification are non trivial and most are failing. For example this is failing for obvious reasons.

  inline I32 get_command(CHAR* string) const { return snprintf(string, sizeof(string), "-%s %g %g %g ", name(), ll_x, ll_y, tile_size); };

Did you do it by hand or did you write an automatic replacement script? If I have to do it by hand I'm probably better to comment most of the code automatically. I'm pretty sure most is useless.

Thanks

@eddelbuettel
Copy link
Contributor Author

I think this may be documented in the r-devel branch of the docs, have you check there rather than in the one from the previous 4.2.2 release? The handy link from RStudio / posit has the devel one prettily formatted: https://rstudio.github.io/r-manuals/

As for the changes, I 'did it by hand' which was a bit crazy but still doable for AsioHeaders, easy for digest and something I am now afraid of for Boost where I have not done it. It is too important a library to make those changes so I push back if they ask.

It is once again a 'we know where they are coming from, and it is the right move in the long run' but still a pain to do 🙄

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Dec 19, 2022

Thank you for your reply. So, I won't be able to release rlas asap. The amount of work to be done is significant.

I greped some stuff and I'd say I can probability do 285/785 occurrence by commenting unused code and 305/785 automatically. It remains 195 occurrences spread in 60.000 lines of code that need more careful investigation.

@eddelbuettel
Copy link
Contributor Author

I hear you. In that case if it were me I might consider a first, narrow, micro release adding the one-line from this PR (and possibly other pending issues) to get CRAN update and then work on the (more painful, more work, but as of right now not urgent snprintf transition. Just a thought.

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Dec 19, 2022

I do agree with that plan but if my submission is rejected by the CRAN I think I won't be in position to negotiate. Anyway, I'm submitting and give you feed back later.

@eddelbuettel
Copy link
Contributor Author

That's a fair point! I have only submitted sprintf fixes recently, and not packages that may be held up.

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Dec 19, 2022

Rejected. I sent an email to negotiate. I mentionned your name to (maybe) get more weight 😉.

@eddelbuettel
Copy link
Contributor Author

Thanks for shepherding 1.6.2 onto CRAN -- much appreciated!

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.

None yet

2 participants