search

lemire / streamvbyte

Fast integer compression in C using the StreamVByte codec
Apache License 2.0
374 stars 37 forks source link

Create cmake target #61

Open victor1234 opened 1 year ago

victor1234 commented 1 year ago
lemire commented 1 year ago

@victor1234 Do you know why it fails under Visual Studio?

victor1234 commented 1 year ago

PR is not complete now, I need 1-2 days

victor1234 commented 1 year ago

@lemire PR is in draft mode now. I'm still working. It's not ready for review.

victor1234 commented 1 year ago

This is a comprehensive PR and I would like to discuss whether these changes are suitable for your project. The main goal is to create modern cmake targets that install correctly and are available through various integration methods, making it easier to use the library and build packages for it. In addition, global variables are not used in modern cmake (they are propagated to the main project when the library is integrated via add_subdirectory()), and all properties should be assigned to each target specifically. As I understand it, all properties ( sanitizer, ASAN_OPTIONS, __restrict ) should only apply to the streamvbyte target and not propagate to the main project when linking. Only _ARM_NEON__ should be propagated to the example.

Please answer these questions:

  1. Do we need pure Makefile support?
  2. I would not mention DCMAKE_INSTALL_PREFIX in the readme at all, as installation defaults to the standard location where the target can easily found by find_package(streamvbyte).
  3. Which of the compilation, linking and definition flags should only be left with streamvbyte, and which should propagate to the entire project that streamvbyte is linked to?
  4. There is no arm support in github CI, how can I check if the PR works correctly and arm optimization enabled on my raspberry pi4?
  5. I would suggest moving all includes into one folder, it would be #include <streamvbyte/streamvbyte.h>. But this will not be compatible with older versions.