dvector
dvector copied to clipboard
BareRose
Removing more compiler warnings
Compiling with -Wall -Wextra -Wpedantic with the DVECTOR_ISOC flag, GCC complains about "unnamed structs/unions". This is a feature of C11, not C99. Compiling as C11, it's all good. However, there are still issues with matrix initialization. If you change this:
#define MAT2_ZERO MAT2({0, 0}, {0, 0})
To this:
#define MAT2_ZERO MAT2(0, 0, 0, 0)
An additional warning is suppressed. But we're still not done. Compare these two cases:
mat4 test1 = {{{1, 0, 0, 0}, {0, 1, 0, 0}, {0, 0, 1, 0}, {0, 0, 0, 1}}};
Versus:
auto test2 = (mat4){{{1, 0, 0, 0}, {0, 1, 0, 0}, {0, 0, 1, 0}, {0, 0, 0, 1}}};
The first compiles without warnings. The second, which dvector currently uses, lets us use C++11 auto, but GCC yields: warning: ISO C++ forbids compound-literals [-Wpedantic]. Compiling this as C11:
mat4 test3 = (mat4){{{1, 0, 0, 0}, {0, 1, 0, 0}, {0, 0, 1, 0}, {0, 0, 0, 1}}};
Yields warning: initializer element is not constant [-Wpedantic]. Removing (mat4), the same as test1, gets rid of the warning.
Before I submit my PR, I want to know if you are okay with removing (vec2), (mat2), etc. casts from dvector, or perhaps I only remove them in the #ifdef DVECTOR_ISOC section starting on line 52, making new versions of the matrix initializers as well.
Initialization Macros
From looking things over, I don't think MAT2_ZERO and such are even meant to have those brackets. the MAT2 macro already adds them. That should definitely be corrected.
These "casts" are actually compound-literals, so technically not casts. I'm pretty sure only C++ complains about them (C90 might, but I don't think we're supporting that anyway?) so if there's a better version of the macros for C++ (like without the casts) we could just #ifdef that in based on the standard flag that is set when compiling in C++ mode.
This is a C library first, and supporting C99/C11 is the primary priority. It's also meant to work as C++, so if it's possible to get rid of a warning in C++ without hurting the C version, it should be done. A perfect example of this is casting all malloc calls, doesn't change anything in C but gets rid of a warning in C++. Removing the compound-literals might change some subtle nuances about typing at compile time, like the naked literal could be assigned to anything with a matching structure, not just a mat4. Maybe this is too obscure, but it should be easy to just redefine the initialization macros for C++ mode.
Types/Swizzles I haven't been working with C much this year, also switched OS so I'm not as used to things. I figured I would rework some of dvector when I get back into C anyway, the types in particular.
The old typedefs are built around C11 and trying to squeeze as many swizzles out of it as possible. Have we done that with the ISOC ones? Perhaps we could get a bit more out of those. Furthermore the C11 swizzles don't cover all possible combinations or even all relevant ones, that's why vec3GetXZ exists. I feel like swizzles are a relevant feature for a vector library, thus should be supported even in ISOC mode. Could also add some that currently don't exist cause you can't do them with unions.
Also I've been meaning to add SSE support to dvector, so if we rework the types we should make sure SSE can be used with them easily. I guess if possible that means a union, or casting them somehow so they can be used as SSE types as directly as possible, investigation pending. One potential pitfall here is that if vec3 becomes 4 floats long so it can be used in 16 byte SSE, the vec4.xyz swizzle would no longer work. Maybe the values just need to be copied into local variables of SSE data types somehow, though SSE is very much about performance so it might be relevant.
Documentation/Testing
I think adding a test.c would be useful, I was going to add one with benchmarks once I add SSE support. Plus having a compile-ready demonstration of the library is very useful for learning how to use it. Also makes testing for warnings easier if you can just regularily compile the test.c in various C modes. The header comment of the test source file could then include instructions as to the warning settings to compile with, plus things like "to test ISOC mode, #define DVECTOR_ISOC and compile with -std=c99 or -Wc99-c11-compat".
We should also add details to the readme about what warnings/versions the library is meant to compile under, so users know when to report warnings popping up. In my own projects I usually use -Werror -Wdouble-promotion -Wwrite-strings -pedantic-errors, expanding this with -Wall -Wall -Wextra -Wpedantic for libraries would make sense. In general the more warnings something can avoid the better. Also things like "C11/C17 supported by default, C99 with DVECTOR_ISOC. Should compile as C++11." should be included.
The extra brackets are not valid, so that's an easy fix (maybe put an extra space between matrix columns for visual separation). After that, the most common warning is "initializer element is not constant" (related to compound literals), which appears in C99 and C11 mode with -Wpedantic. Detecting compiler flags won't fix it. This test case produces it:
#define DVECTOR_ISOC
#include "dvector.h"
vec3 test_vec = VEC3_ZERO;
int main(){return 0;}
the naked literal could be assigned to anything with a matching structure
Yes, but I'd rather get rid of the warning, which affects both C and C++. dvector functions are strictly typed with no type deduction, unlike linalg.h. What problem would you see occuring?
The old typedefs are built around C11 and trying to squeeze as many swizzles out of it as possible. Have we done that with the ISOC ones? Perhaps we could get a bit more out of those.
The ISOC unions are currently minimalist. You can add that if you want, but I don't need it. I understand it makes the code run faster, but I don't care too much about speed when it comes to game math on the CPU. A physics engine could benefit from enhanced speed though. The same sentiment also applies to SSE.
From where I stand, tests are important, and compactness is another priority. Correctness and readability over speed, unless there is a compelling reason to optimize for speed.
Yea, some better visual indication what columns/rows are would be nice, maybe a comment too. It's not just the MAT2_ZERO macros that seem to have excess brackets, perhaps we can cut down on the ones in the function-like initializer macros aswell.
The C++ specific warning I was referring to is warning: ISO C++ forbids compound-literals [-Wpedantic], but it seems like the literals cause problems even in C so that point is moot. Looks like the initializer warning has to do with test_vec having static storage duration.
What problem would you see occuring?
It's nothing big, but the compound-literals prevent stuff like this from compiling:
mat2 m2 = VEC4_ZERO;
Being able to assign a vec4 to mat2 feels broken, and could theoretically cause weird bugs, at least in more complex code. If we reduce the macros to only the minimal curly brackets (one at the start and one at the end) this works the other way around too.
I'll look into how essential the literals actually are, and if there are better alternatives.
The ISOC unions are currently minimalist. You can add that if you want, but I don't need it.
So you don't use swizzles much them? Personally I rarely use most of the union swizzles, and while I think a vector library should have swizzles, they don't strictly need to be unions. The performance benefit is pretty fringe anyway. If the ISOC version can be expanded a bit we could just unify the ISOC and C11 versions and have the few most important swizzles as functions. Would simplify things for sure.
I understand it makes the code run faster, but I don't care too much about speed when it comes to game math on the CPU. A physics engine could benefit from enhanced speed though. The same sentiment also applies to SSE.
A low-level vector library is likely to be used in performance-critical code, and probably in a lot of places in the code. How do you personally do graphics? Just use a pre-existing graphics/game engine or some larger library/framework like SDL? I made my own basic graphics library using the OpenGL API, which uses dvector to prepare values to send to the GPU and do stuff like frustum culling.
I will look into SSE more at some point. If adding the SSEs to dvector would cause too much clutter (like a whole new version of every function for SSE), would you prefer if I made it a seperate file in this repo, its own repo, or put it in the dvector header even if it clutters it up? The goal would be to keep the API the same either way, so compiling with/without SSE is as easy as swapping out dvector for its SSE version. Well-implemented SSEs can bring the kind of performance boots that are relevant to any use case, too. If something is twice as fast that's beneficial even if performance isn't critical.
Turns out the compound-literals are necessary for the macros to work as function arguments, which is most of the reason those macros exist. Removing the compounds also breaks a bunch of dvector's own code, meaning it would break existing application code and is therefore a no-go. Try removing the (vec3) part from VEC3 and compiling this:
#define DVECTOR_STATIC
#include "dvector.h"
int main () {return 0;}
Seems like C expects the compound literals in a couple places where naked literals won't work. AFAIK the compounds only produce a warning when initializing variables with static storage duration anyway, and are fine everywhere else (except maybe in C++).
For the static storage duration initialization I would propose alternate versions of the macros suffixed with INIT like so:
#define VEC2_INIT(X, Y) {X, Y}
#define VEC2_ZERO_INIT VEC2_INIT(0, 0)
In the special case of static variables these new macros can be used to avoid the warning, with the old ones working everywhere else.
Strictly speaking we don't even need new macros, the point of them is to simplify making compound literals, naked literals are less code than a macro anyway. We could just add a comment warning about static initialization and recommending the use of naked literals, so instead of VEC3(1, 2, 3) you write {1, 2, 3}. I guess they're still more readable than naked literals, and the constants are still useful.
~~Another possible solution:~~
~~static const vec2 VEC2_ZERO = {0, 0};~~
~~Seems to fix common issues, leaving it up to the compiler to either inline the literal or treat it as a variable. Only problem is the "defined but not used" warning, which could be hacked around.~~
(EDIT: Seriously, don't do that.)
The INIT suffix would be less hack-ish, but naked literals work too. When you have to do mat4 literals you have a few instances of {1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1}. I've written far uglier things, so naked literals work for me, though a mention in a source comment somewhere would be nice.
If adding the SSEs to dvector would cause too much clutter (like a whole new version of every function for SSE), would you prefer if I made it a seperate file in this repo, its own repo, or put it in the dvector header even if it clutters it up?
I would prefer a separate file in the same repo, like dvector_simd.h or dvector_sse.h. When files exceed 1000 lines and their original authors die or change careers, novice programmers can't make sense of the code. I think it would be more readable this way.
So you don't use swizzles much them? Personally I rarely use most of the union swizzles, and while I think a vector library should have swizzles, they don't strictly need to be unions. The performance benefit is pretty fringe anyway. If the ISOC version can be expanded a bit we could just unify the ISOC and C11 versions and have the few most important swizzles as functions. Would simplify things for sure.
I thought there already were swizzle functions, but I hardly use them, as manually specifying things like {vec.x, vec.y, vec.z} works just as well.
Vector math libraries are used in "hot" code, so I do understand that if this is the basis for anything non-trivial it's important to optimize. I'll leave the "how" to you, since I'm in a bit of a time crunch this week and next. Thanks for playing along. :)
I'm happy enough with my PR for now. If you want to merge it, I may use dvector in a public project. Not trying to pressure you or anything, but warning-free is one of my goals. :)