mklarqvist / libalgebra Goto Github PK

First, awesome project. I already see some ideas I want to try out in my own code.

Just taking a quick look through the code I noticed some things I would have done differently. Not bugs, but a few suggestions… feel free to ignore this if you want.

• I have more robust versions of several macros you use in Hedley which might be worth a look:
  • STORM_RESTRICT → HEDLEY_RESTRICT
  • STORM_FORCE_INLINE → HEDLEY_ALWAYS_INLINE
  • STORM_NO_INLINE → HEDLEY_NEVER_INLINE
  • WARN_UNUSED → HEDLEY_WARN_UNUSED_RESULT
• For WARN_UNUSED, I think you forgot the STORM_ prefix.
• For __has_attribute and __has_builtin, please define STORM_HAS_* or something instead. Using __has_ can cause problems for other people.
• It looks like you know this already, but CLANG_PREREQ is dangerous. https://sourceforge.net/p/predef/wiki/Compilers/#clang has an overview of the problem. I see you're checking __apple_build_version__ sometimes, too, but Apple isn't the only compiler that defines __clang__ and friends. For AVX2/AVX512, maybe you could use __has_builtin on the intrinsic(s) you're worried about?
• Maybe a macro for __attribute__ ((target ("sse4.2"))) instead of checking for _MSC_VER every time?
• Is there a reason to use uint64_t instead of size_t for buffer lengths? It's not really a problem, size_t is just the more idiomatic choice…
• It would be nice to see conformant array parameters, at least in the public API. It would require putting the size before the data parameter, but I think it's a worthwhile change. Hedley has a macro if you want to copy it: HEDLEY_ARRAY_PARAM, or you could just unconditionally define it to nothing and you only get the documentation benefits. TBH, compilers and static analyzers aren't (yet?) great about taking advantage anyways ☹
• GCC supports __builtin_popcount back to 3.4. Also, armcc 4.1+ and XL C/C++ 13.1.5+. (I didn't check armcc or xlc, either…)

For the Hedley macros, if you don't want to copy over the logic (which can be a huge pain; see the introduction in the Hedley User Guide…), you could make it an optional dependency:

#if defined(HEDLEY_RESTRICT)
#  define STORM_RESTRICT HEDLEY_RESTRICT
#elif defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
#  define STORM_RESTRICT   restrict
#else
/* note : it might be useful to define __restrict or STORM_RESTRICT for some C++ compilers */
#  define STORM_RESTRICT   /* disable */
#endif

I do that a lot in portable-snippets and I think it's a good compromise. People who want to can include hedley.h, everyone else just gets simpler fallbacks.

i downloaded Apex Legends and it showed your PC does not have popcnt...I googled about it and they redirected me to this page, how the hell do i download it?

Dear Marcus,

Is it possible to change the length types to uint64_t so that the algorithms work for very long arrays as well (~2^35 of 8-byte words)?

I found another library https://github.com/kimwalisch/libpopcnt, where they use uint64_t, but unfortunately, it doesn't have your nice functions for computing intersection and union.