Follow up on pr1146 (v2)#1221
Conversation
mgates3
left a comment
There was a problem hiding this comment.
Looks fine. Maybe a couple things could be improved later on.
LAPACKE/src/lapacke_cgesvdq_work.c
Outdated
| API_SUFFIX(LAPACKE_lsame)( jobv, 'r' ) ) { | ||
| LAPACKE_free( v_t ); | ||
| } | ||
| if ( ( m > 0 ) && ( n > 0 ) ) LAPACKE_free( v_t ); |
There was a problem hiding this comment.
Ok, but since free( NULL ) does nothing, it would be simpler to have just one exit level and free all the pointers:
exit_level_0:
LAPACKE_free( v_t ); v_t = NULL;
LAPACKE_free( u_t ); u_t = NULL;
LAPACKE_free( a_t ); a_t = NULL;
I tend to reset pointers to NULL after freeing them, to avoid any possible confusion.
If desired, perhaps that can be a future simplification of all of LAPACKE.
There was a problem hiding this comment.
I am slowing down on that, so, as you suggest: "could be improved later on." So, if we get there, for sure, let us do a separate pull request.
I do like the idea to reset pointers to NULL after freeing them, to avoid any possible confusion.
Here is why I am slowing down: While free(NULL) does nothing, is it true that LAPACKE_free( NULL ) does nothing? What do we ask LAPACKE_free() to do? This proposed change would implicitly state that we require LAPACKE_free( NULL ) to do nothing.
I like the logic "if you fail to allocate at level 2, then do not forget to free level 0 and level 1 that were previously allocated." I feel it could be useful in some context, but maybe not for a C wrapper.
So all in all, are there more opinions about what @mgates3 suggest? In favor? Against?
…workspace would be helpful. (Per later comment when transposing results.)"
…freeing them, to avoid any possible confusion."
Follow up on PR #1146. ( Thanks to @yizeyi18 for the PR.)
While reviewing, @angsch suggested
I implemented the suggestion. This ended being easier for me to do a substantial rewrite.
I followed the explanation at
lapack/SRC/cgesvdq.f
Lines 159 to 198 in 034ada1