Skip to content

Remove BLAS.h, Linpack.h and Lapack.h from bindings#197

Closed
CGMossa wants to merge 8 commits intomasterfrom
remove_blas_lapack
Closed

Remove BLAS.h, Linpack.h and Lapack.h from bindings#197
CGMossa wants to merge 8 commits intomasterfrom
remove_blas_lapack

Conversation

@CGMossa
Copy link
Member

@CGMossa CGMossa commented Nov 12, 2023

Inspired from a news item

Headers ‘R_ext/Applic.h’ and ‘R-ext/Linpack.h’ used to include ‘R_ext/BLAS.h’ although this was undocumented and unneeded by their documented entry points. They no longer do so.

We actually don't use the wrappers to BLAS.h, Lapack.h or even Linpack.h, so we could remove them and improve clarity of bindings. If we were to include them, we'd actually need to write wrappers on top of them, as they aren't directly applicable.

Diff on removing:

@CGMossa
Copy link
Member Author

CGMossa commented Nov 12, 2023

/bindings

@CGMossa
Copy link
Member Author

CGMossa commented Nov 12, 2023

/bindings

@CGMossa
Copy link
Member Author

CGMossa commented Nov 12, 2023

/bindings

@yutannihilation
Copy link
Contributor

Just in case you don't notice.

  • I believe libR-sys doesn't use R-ext/Linpack.h, so you don't need to exclude it.
  • libR-sys does use R_ext/Applic.h, and the BLAS removal was already reflected by Update bindings #182. But, this happens only on R-devel, and I have no strong opinion on removing them from the prior versions as well.

Also, I'm feeling this reasoning is not very true. These BLAS functions are independent from SEXP, so I don't think it's extendr's responsibility to provide such wrappers anyway.

We actually don't use the wrappers to BLAS.h, Lapack.h or even Linpack.h, so we could remove them and improve clarity of bindings. If we were to include them, we'd actually need to write wrappers on top of them, as they aren't directly applicable.

Personally, I'd vote for removing R_ext/Applic.h.

@CGMossa
Copy link
Member Author

CGMossa commented Nov 12, 2023

Thanks! As always, very good feedback! So I wanted to make a "Remove Applic.h" next, but I remembered why (apparently I added it), it is because it contains some routines that aren't Fortran ones. Maybe #199 is a better solution to this?

@CGMossa
Copy link
Member Author

CGMossa commented Nov 12, 2023

Closed in favour of #199

@CGMossa CGMossa closed this Nov 12, 2023
@CGMossa CGMossa deleted the remove_blas_lapack branch April 30, 2024 14:16
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