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

Const correctness2 #15

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Const correctness2 #15

wants to merge 5 commits into from

Conversation

AerysL
Copy link
Collaborator

@AerysL AerysL commented Nov 27, 2018

This improves the const correctness of EVSL. Useful for reasons such as

It protects you from accidentally changing variables that aren't intended be changed,
It protects you from making accidental variable assignments, and
The compiler can optimize it. 
For instance, you are protected from
if( x = y ) // whoops, meant if( x == y )

as per https://stackoverflow.com/questions/136880/sell-me-on-const-correctness

@AerysL AerysL requested a review from liruipeng November 27, 2018 03:14
@liruipeng
Copy link
Collaborator

@AerysL This makes sense to me. We did not pay much attention on the use of keyword "const".

It seems like C++ is more strict on this than C. E.g., C allows mismatch between "const type *" and "type *" used in function prototypes and functions, but C++ does not.

Please try to build with C++. Do "./configure --with-cxxcompiler" and "make -j test", and check if there is something missed.

Nice work. Thanks!

@AerysL
Copy link
Collaborator Author

AerysL commented Dec 3, 2018

Good catch, the errors specifically are due to the fact that the reference BLAS implementation (and apparently some other BLAS implementations as well) don't declare const correctness correctly. I used the definitions used by MKL's implementation (which appears to be the same as the OpenBLAS implemenation).
The way to handle this is to use C++'s const_cast. I think the best way to handle this is probably to provide a section which, when compiled with evsl, defines evsl_blas to be a wrapper functions around the desired blas call with const_cast applied correctly. Does this sound reasonable to you?

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