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

remove redundant space from xerbla call sbgv/hbgv #792

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

dklyuchinskiy
Copy link
Contributor

Part of issue #791.

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (f6c9536) compared to base (1fafb88).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head f6c9536 differs from pull request most recent head 9f7c029. Consider uploading reports for the commit 9f7c029 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #792   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files        1908     1908           
  Lines      187072   187072           
=======================================
  Misses     187072   187072           
Impacted Files Coverage Δ
SRC/chbgv.f 0.00% <0.00%> (ø)
SRC/dsbgv.f 0.00% <0.00%> (ø)
SRC/ssbgv.f 0.00% <0.00%> (ø)
SRC/zhbgv.f 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@langou
Copy link
Contributor

langou commented Feb 8, 2023

Thanks. I do not have a strong opinion about these white spaces. Either way is fine. Thanks for the PR. That's great.

@langou langou merged commit 10abd46 into Reference-LAPACK:master Feb 8, 2023
@dklyuchinskiy
Copy link
Contributor Author

dklyuchinskiy commented Feb 8, 2023

Thanks. I do not have a strong opinion about these white spaces. Either way is fine. Thanks for the PR. That's great.

I just wrote a small unit test to see the error message, which xerbla returns, and it is failed due to additional space in function name. Now behaviour is aligned with other LAPACK functions.

Thanks for the merge!

@langou
Copy link
Contributor

langou commented Feb 8, 2023

Ah! OK. Thanks for letting me know. Perfect then.

@martin-frbg
Copy link
Collaborator

Actually many (most?) five-letter functions that use XERBLA have this trailing space, maybe it is the unit test that needs to be more flexible ?

@dklyuchinskiy
Copy link
Contributor Author

dklyuchinskiy commented Feb 8, 2023

Actually many (most?) five-letter functions that use XERBLA have this trailing space, maybe it is the unit test that needs to be more flexible ?

OMG, indeed. I am sorry, I did not check other 5 letter functions... Is it done for special purpose? LAPACK namings can be more than 6 letters.

Yeap, unit test can be changed, but it should be? Unfortunately, trailing space led to misundersanding from my side. It could be reverted if there exists LAPACK standard in such case. But for my point of view it is not aligned between functions.

@langou
Copy link
Contributor

langou commented Feb 8, 2023

Is it done for special purpose?

No, this is a remnant of FORTRAN 77. Six-letter is not needed any longer. The space in the parameter passed to XERBLA is not needed any longer.

The code with a space CHBGV is OK. The code without a space is OK too. I am fine either way.

XERBLA should work with a space in the name of the subroutine. So it does bother me that the code with a space is a problem on your end. That being said, we can remove the space. I do not mind.

LAPACK namings can be more than 6 letters.

I agree with @martin-frbg that we want to be consistent across 5-letter-name subroutines.

@dklyuchinskiy
Copy link
Contributor Author

@langou thank you for comprehensive explanation!

Of course, it would be nice to have the same style for all 5-letter-name subroutines. Indeed, not all are aligned. For example, GEQR, GELQ, and now SBGV 😅 If it is not a big problem at the moment, it can be kept as is.

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