search

aklomp / base64

Fast Base64 stream encoder/decoder in C99, with SIMD acceleration
BSD 2-Clause "Simplified" License
884 stars 165 forks source link

Replace custom platform detection logic with Boost.Predef #13

Open BurningEnlightenment opened 8 years ago

BurningEnlightenment commented 8 years ago

Hi, Proposal: Replace all the custom preprocessor checks for architecture, platform, compiler, SIMD, etc. with the Boost.Predef library which is independent and header only, so that one can simply import the necessary header files into the repository and be done with it (please note that this is an internal change and doesn't affect library users in any way). I think that this will improve code readability, because the Boost.Predef macros are far more descriptive and might prevent fallacies like the one in #12, e.g. compare

#ifdef BOOST_COMP_MSVC_AVAILABLE
#ifdef BOOST_OS_WINDOWS_AVAILABLE

with

#ifdef _MSC_VER
#if defined(_WIN32) || defined(__WIN32__)

If you think that this is a good idea, but don't have the time to make the change I'll happily submit a pull request.

Cheers, Henrik

aklomp commented 8 years ago

First off, there may be a license conflict: Boost.Predef is licensed under the Boost license, whereas this project is BSD 2-clause. Maybe that's not a problem if we ship an unmodified version of the original, but I'd like to be sure. Is Boost.Predef a standalone project or an integral part of Boost? If I include it as a dependency, I'd prefer to have a properly packaged third-party lib instead of some loose files copied out of a tarball.

My opinion is that the Base64 library should in principle be self-contained, without any third party code. Rationale: I don't want to maintain third party code if I don't have to, and I'd prefer to keep the scope of this library small. (Including lots of extra code automatically pulls in... lots of extra code.) However, I must admit that at this point, the Base64 library does a lot of platform-dependent, nonportable checks (example: the whole build system), so it might not be the worst idea to delegate that to an industrial-strength solution. I must admit that the docs make it look like this would simplify a lot of things, which is a win. How do you think this would integrate with the proposed Cmake solution? Could Cmake re-use the Boost defines, or would it still run its own platform detection? It would be really clean if we could merge all the platform stuff into one module.

BurningEnlightenment commented 8 years ago

OK, so let's get the licensing issue settled first:

Boost.Predef is licensed under the Boost license, whereas this project is BSD 2-clause. Maybe that's not a problem if we ship an unmodified version of the original, but I'd like to be sure.

The boost license basically says that we have the permission to use, reproduce, display [and] distribute [...] the Software (which is what we want to do) as long as we retain the copyright notices and distribute a copy of the license along with the source code. So if we included (parts of) the library, we would put a Boost.License file in that directory and be done with it. The users of this library however aren't affected by this in any way because the license explicitly makes an exception for compiled software allowing use and redistribution without any restrictions. To sum it up: If a user copied the source tree he would also copy the Boost.License within the source tree and thereby comply with the license. If a user compiled this library and linked against it he would be eligible for the license exception and therefore be compliant with the license. (Please note that in addition to that the terms of the BSD-License of this project still apply and must be complied with)

Is Boost.Predef a standalone project or an integral part of Boost?

Well, actually it is both :smile:. Many Boost libraries build upon those preprocessor checks and it is included in the main Boost package. Therefore I would call it an integral part. On the other hand it doesn't depend on anything but the C Preprocessor and has its own github repository, i.e. it is well suited for use outside of the Boost context without pulling in any other library or source file.

My opinion is that the Base64 library should in principle be self-contained, without any third party code. Rationale: I don't want to maintain third party code if I don't have to, and I'd prefer to keep the scope of this library small. (Including lots of extra code automatically pulls in... lots of extra code.)

I suggest that we include the Boost.Predef library through the means of a git submodule hook targetting their official github repository. That way you can maintain a clear seperation between Boost.Predef and your project plus getting all the benefits of using the Boost.Predef library. Boost.Predef issues would be reported to their repository and addressed over there which may even reduce the amount of code you have to maintain.

How do you think this would integrate with the proposed Cmake solution? Could Cmake re-use the Boost defines, or would it still run its own platform detection? It would be really clean if we could merge all the platform stuff into one module.

While one cannot use Boost.Predef stuff directly from CMake, it is indeed possible to build a very clean and lean platform detection module on top of Boost.Predef.

HTH