microsoft / ALEX

A library for building an in-memory, Adaptive Learned indEX
MIT License
662 stars 114 forks source link

Allow compilation outside Visual Studio #28

Closed MrPekar98 closed 2 years ago

MrPekar98 commented 2 years ago

A couple of files are missing the inline and constexpr keywords. Without these, multiple definitions exist whenever alex.h is included more than once.

ghost commented 2 years ago

CLA assistant check
All CLA requirements met.

jialinding commented 2 years ago

Thanks for the PR! Could you explain why the inline keyword is necessary? My understanding is that inline is just a hint and shouldn't cause compilation errors.

MrPekar98 commented 2 years ago

Of course. The inline keyword has two functions: it can be used in function declarations to give the compiler a hint to substitute each call to that function with its body, or it can be used to allow for multiple definitions of that function or variable. The latter is the reason it is necessary. For example, if one would use your library in a project consisting of a single file, everything would be fine because the function definitions are included once and therefore defined once. However, in a large project of multiple .cpp files that will be linked, multiple definitions of those functions would exist, one per .cpp file where the library is included/preprocessed.

If I'm not mistaken, the MSVC C++ compiler is able to "inject" the inline keyword whenever necessary, hence allowing the programmer to ignore using the keyword. However, other compilers, such as GNU and Clang, are not able to do so.

The inline keyword can be omitted for class/struct functions because it's already implicit. I added the constexpr keyword to globally defined variables because they are constants and because the constexpr keyword additionally has the same effect as the inline keyword.

Here is a great source about the inline keyword. It is specifically point 2.1 in the Explanation section that is important in this case.

jialinding commented 2 years ago

Thanks for the explanation! Everything looks good to me.