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

submit changes necessary for cran #2094

Open
wants to merge 2 commits into
base: latest
Choose a base branch
from

Conversation

FlorianSchwendinger
Copy link

@FlorianSchwendinger FlorianSchwendinger commented Dec 25, 2024

The repo for R packages (CRAN) has very strict requirements (enforced by custom checks) which have to be fully filled otherwise the package is removed from CRAN.

The following small changes are required from CRAN currently I apply them at each update. It would be nice if you could apply the changes upstream since it would simplify the maintenance of the R package and the changes are quite simple.

  1. In the files
  • extern/filereaderlp/reader.cpp and
  • extern/filereaderlp/reader.hpp
    CRLF line endings are replaced with CR to address the CRAN warning
    "Found the following sources/headers with CR or CRLF line endings".
  1. Changed
    void PDHG_PrintUserParamHelper() to void PDHG_PrintUserParamHelper(void) and
    void PDHG_PrintHugeCUPDHG() to void PDHG_PrintHugeCUPDHG(void)
    to address the CRAN warning
    "function declaration isn't a prototype"

  2. Added a newline at several files to address the CRAN warning
    "Found the following sources/headers not terminated with a newline:"

@jajhall
Copy link
Member

jajhall commented Dec 26, 2024

Thanks, I'll look at these in due course, but I'm on holiday without access to a Linux machine

@jajhall jajhall self-requested a review December 26, 2024 10:42
@jajhall jajhall changed the base branch from master to latest December 26, 2024 10:43
@jajhall jajhall requested a review from galabovaa December 26, 2024 10:47
Copy link
Member

@jajhall jajhall left a comment

Choose a reason for hiding this comment

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

I now see properly what the changes are. The CRLF to CR changes are uncontroversial. Do they come from editing on Windows creating the "^M" that's seen when viewing files on a Linux editor?

I'll leave the PDLP modifications to @galabovaa, but I guess they are fine

@FlorianSchwendinger
Copy link
Author

Thank you!

I now see properly what the changes are. The CRLF to CR changes are uncontroversial. Do they come from editing on Windows creating the "^M" that's seen when viewing files on a Linux editor?

Yes exactly.

I'll leave the PDLP modifications to @galabovaa, but I guess they are fine

You can reproduce the warnings by adding the flag -Wstrict-prototypes to the C compiler.
This link gives detailed information on this topic and references to the corresponding sections in the C and C++ Standard.

Copy link
Contributor

@galabovaa galabovaa left a comment

Choose a reason for hiding this comment

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

Looks good to me!

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.

3 participants