Update lapacke related C type usage #182
Conversation
WalkthroughThe changes primarily focus on modifying the type and syntax of certain function parameters across multiple files. The alterations ensure better type compatibility and cleaner code, enhancing the overall readability and maintainability of the codebase. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- la/matrix_ops.v (1 hunks)
- vlas/conversions.v (1 hunks)
- vlas/lapack.v (3 hunks)
Files skipped from review due to trivial changes (1)
- la/matrix_ops.v
Additional comments (Suppressed): 7
vlas/lapack.v (5)
8-8: The function signature for
C.LAPACKE_dgesvdhas been updated to replacebyteparameters with&charparameters. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.16-16: The function signature for
C.LAPACKE_dgeevhas been updated to replacebyteparameters with&charparameters. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.18-18: The function signature for
C.LAPACKE_dlangehas been updated to replace thebyteparameter with a&charparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.79-82: The function
dgesvdhas been updated to take&charparameters instead ofbyteparameters. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.202-204: The function
dlangehas been updated to take a&charparameter instead of abyteparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.vlas/conversions.v (2)
23-24: The function
l_uplohas been refactored to use a ternary operator instead of an if-else statement. This change improves readability and reduces the number of lines of code. However, ensure that the ternary operator is well supported and behaves as expected in your language and environment.27-32: The function
job_vlrnow returns a reference to acharinstead of au8. This change likely improves type compatibility with the updated LAPACKE functions. However, ensure that all calls tojob_vlrthroughout the codebase have been updated to handle the new return type.
| @@ -199,6 +199,6 @@ pub fn dgeev(calc_vl bool, calc_vr bool, n int, mut a []f64, lda int, wr []f64, | |||
| } | |||
| } | |||
|
|
|||
| pub fn dlange(norm byte, m int, n int, a []f64, lda int, work []f64) f64 { | |||
| pub fn dlange(norm &char, m int, n int, a []f64, lda int, work []f64) f64 { | |||
There was a problem hiding this comment.
what happens if you just use str instead of &char? I think V handles that when doing interop with C but I'm not totally sure
There was a problem hiding this comment.
that should simplify the code because we can use str everywhere instead of casting to &char
There was a problem hiding this comment.
Good point. The cast to char should be handled internally. The below would work:
pub fn dlange(norm string, m int, n int, a []f64, lda int, work []f64) f64 {
return unsafe { C.LAPACKE_dlange(.row_major, &char(norm.str), m, n, &a[0], lda, &work[0]) }The cast needs to be explicit. Just norm.str only compiles without -cstrict. Maybe this changes when the V compiler evolves. Until then &char(norm.str) is the proper usage, which is okay for internal code I guess. (I'm gonna open an issue at V regarding this, as I need to use this cast a lot in my own code bases as well when interoping with C. Just .str would add some convenience).
Regarding the norm type, it depends if string is the best. If norms are set by a single character rune would be better. I'm not too familiar with the usage of lapacke yet and can't tell if an enum is appropriate for the norms. In any case, the cast to char could and should be handled internally.
Which one should it be?
There was a problem hiding this comment.
OK, makes sense! Will Merge now!
There was a problem hiding this comment.
Thanks @ulises-jeremias !
I think t was a valid point and that it's better to use &char only in the C functions.
That's why I thought it might be improved and ended the comment with a question.
So for pub fn dlange I'm changing it to rune in #184. Than it's the same usage as before for this public function. Just with internal C type handling corrected.
Regarding .str cast situation, opened an issue with a minimal repro: vlang/v#19623
🙂:+1
Summary by CodeRabbit
l_uplofunction for improved readability and maintainability.These changes are internal and do not introduce new features or directly affect user experience. They contribute to the overall robustness and maintainability of the codebase.