sgorsten / linalg

linalg.h is a single header, public domain, short vector math library for C++
The Unlicense
854 stars 68 forks source link

Undefined behavior on operator[] overload prevents use in constexpr context #13

Closed mirmik closed 5 years ago

mirmik commented 6 years ago

Good time of day. For start, my applause to this great library.

I've done some experiments with the code and I want to share a few considerations. An attempt to create a general form of mat and vec is complicated for compat with constexpr semantic in current library form.

    constexpr const T & operator[] (int i) const { return (&x)[i]; }

It isn't really constexpr and that is very sad, becourse we cann't use access in general form's constexpr method.

I intended to offer change declaration

    T x,y;

to

    union { std::array<T,2> a; struct { T x,y; }; };

with

    T & operator[] (int i) { return a[i]; }

for support general constexpr access. But, for some reason of current implementation of constexp in c++, we can not effectively use initialization through x, y, z, and access through an array.

I tried to rewrite the entire library to work through an array. The result is here https://github.com/Mirmik/linalg/blob/master/linalg.h. This is a slightly more ugly solution, but in the context of it we can create a comfortable general form mat and vec (sketch here: https://github.com/Mirmik/linalg/blob/master/linalg_ext.h).

What do you think about it all?

sgorsten commented 6 years ago

My apologies for the late response. You are correct that the array access operator is not constexpr. In fact, it's straight-up undefined behavior, and only works by chance. Unfortunately, the proposed union-based approach ALSO relies on undefined behavior, which is why it has usability issues in a constexpr context.

Resolving this is a priority for me and I am most likely to adopt t0rakka's approach to solving this problem (https://t0rakka.silvrback.com/simd-scalar-accessor). This is probably a breaking change for some users, but I intend to work on a v3.0 over the next few months, and this fix will probably make it into v3.0.

mirmik commented 6 years ago

simd-scalar-accessor looks as good solution

sgorsten commented 5 years ago

Fixed in v3 branch as of 7357fb2b76f619e8136e7d9fc40a87ba0f0603b4