Martinsos / edlib

Lightweight, super fast C/C++ (& Python) library for sequence alignment using edit (Levenshtein) distance.
http://martinsos.github.io/edlib
MIT License
493 stars 162 forks source link

Fix `-Wold-style-cast` #133

Closed SoapZA closed 4 years ago

SoapZA commented 4 years ago

Hi @Martinsos, Thanks for a great library! In our build systems, we always add -Wold-style-cast, in order to avoid C-style casts in C++. This PR is about using C++'s static_cast consistently in edlib. static_cast has a number of advantages and is recommended by the C++ Core Guidelines

void fun1(char* x)
{
    int* i = (int*) x;
    int* j = static_cast<int*>(x);
}

the C-style cast succeeds, whereas the static_cast would cause the compiler to error out, for good reason (undefined behavior, likely violates strict aliasing and alignment requirements).

Martinsos commented 4 years ago

Hi @SoapZA , thanks for the PR :)! Sounds great, I will take proper look in the next few days when I catch some time and will get back to you then.

SoapZA commented 4 years ago

@Martinsos I've added some touchups for CMake as well that make packaging easier for distros, because it allows them to change the paths (on 64-bit Linux, you need to install libraries into lib64 and not lib). By using GNUInstallDirs, most distros can transparently change the defaults (which haven't changed) using their own build scripts. Furthermore, by using CTest users can also disable building unit tests by calling CMake with -DBUILD_TESTING=OFF. The defaults are unaffected by this, unit tests are built like before.

SoapZA commented 4 years ago

@Martinsos I have also fixed -Wshadow. In general, relying on shadowing variables is not good, see this part by Louis Brandy why -Wshadow can catch really nasty bugs: https://youtu.be/lkgszkPnV8g?t=1752

Martinsos commented 4 years ago

@SoapZA Great! All looks good, thanks for all the information, I learned a lot. If you have any other suggestion feel free to make those also, and I will merge now what you have done so far.