7
\$\begingroup\$

I recently started working on my own vector library for a project and I thought it would be a good idea to get some insight on the code that I have written so far.

It's very simple as I'm new to C.

Vector.h

#ifndef _VECTORS_H
#define _VECTORS_H

#define ABS(n) (((n) < 0) ? -(n) : (n)) /* Absolute function */
#define TOL 0.001 /* Tolerance */

#include "t_math.h"

typedef struct
{
    float x;
    float y;
}Vector2D;



/*
    Get length of 2D vector
    return: Float
*/
float GetVector2DLength(Vector2D a)
{
    float v_length = v_sqrt(a.x*a.x+a.y*a.y);
    return v_length;
}

/*
    Get unit of 2D vector
    return: Vector2D
*/
Vector2D UnitVector2D(Vector2D a)
{
    float v_length = GetVector2DLength(a);

    float new_x = a.x / v_length;
    float new_y = a.y / v_length;

    Vector2D n_Vector = {new_x, new_y};

    return n_Vector;
}

Vector2D Vector2DSubtract(Vector2D a, Vector2D b)
{
    Vector2D v;
    v.x = a.x - b.x;
    v.y = a.y - b.y;
    return v;
}

float Vector2D_DotProduct(Vector2D a, Vector2D b)
{
    float dp = a.x*b.x+a.y*b.y;
    return dp;
}


#endif

t_math.h

#ifndef _TRASH_MATH_H
#define _TRASH_MATH_H


/*
    Newton-Raphson method of square root
    code taken from: vs97 on stackoverflow
    Retuns: Float
*/

float v_sqrt(float x)
{
    float y = 1.0;
    while(ABS(x/y - y) > TOL)
    {
        y = (y+x/y)/2.0;
    }
    return y;
}



#endif

Be as ruthless as you need to be, as I'm trying to better my C coding abilities.

\$\endgroup\$
3
  • \$\begingroup\$ Shouldn't functions that return Vector2D s allocate them on the heap? \$\endgroup\$
    – lhballoti
    Feb 15 at 15:38
  • 2
    \$\begingroup\$ @lhballoti: Why in the world would you do that? This datatype is plenty small enough to pass by value efficiently, stack allocation is much more efficient than heap, and one will very often want to define arrays with memory locality, not arrays of pointers into the heap. \$\endgroup\$
    – Ben Voigt
    Feb 15 at 21:53
  • \$\begingroup\$ You're right. I'm so used to handling arrays and pointers that I forgot complex types can be passed by value as well. \$\endgroup\$
    – lhballoti
    Feb 15 at 22:43

4 Answers 4

11
\$\begingroup\$

Identifers beginning with an underscore and a capital letter are reserved by the compiler and standard library maintainers. So, technically, _VECTORS_H and _TRASH_MATH_H is undefined behavior. See this link for more info.


You should define ABS and TOL where they are needed i.e., in t_math.h.

  1. It's good practice, keep associated functions and symbols close to each other.

  2. Your current implementation relies on a particular order of includes; users of your library will now be forced to include vector.h to use t_math.h, that too in a specific order (vector.h before t_math.h), regardless of whether they need vector.h or not.


Be consistent with naming. Why are identifiers in vector.h defined in PascalCase while the identifiers in t_math.h defined in snake_case?


This one is personal preference, but I'd get rid of most of the comments. GetVector2DLength is descriptive enough; the comment is just visual noise.


The biggest issue with your implementation is the fact that your functions will be defined in every translation unit. If I compile a project with multiple translation units including vector.h or t_math.h, it will fail because the linker will see multiple definitions of the functions and won't know which one to pick.

You can fix this by either:

  • Declaring your functions static:

    static float GetVector2DLength(Vector2D a)
    {
        float v_length = v_sqrt(a.x*a.x+a.y*a.y);
        return v_length;
    }
    

    This will make sure your functions have internal linkage (i.e., not visible to other translation units). However, this has an adverse effect that it would increase the binary size and any changes to the header files means all translation units including this file will need to be recompiled; this can substantially increase your build times.

  • The preferred and most common option is to only declare the functions in the header and define them in their own translation unit:

    vector.h

    #ifndef vector_h
    #define vector_h
    
    float GetVector2DLength(Vector2D a);
    
    #endif vector_h
    

    vector.c

    #include "vector.h"
    
    float GetVector2DLength(Vector2D a)
    {
        float v_length = v_sqrt(a.x*a.x+a.y*a.y);
        return v_length;
    }
    

    One thing to keep in mind is that headers should ideally only declare the public interface, and internal functions can still be defined as static inside the translation unit.

\$\endgroup\$
2
  • \$\begingroup\$ This will make sure your functions have internal linkage (i.e., not visible to other translation units). However, this has an adverse effect that it would increase the binary size Does that mean that static functions included in multiple translation units are duplicated for each one they are included in? \$\endgroup\$ Feb 14 at 17:21
  • 1
    \$\begingroup\$ @TomTsagkatos: Yes they are. The linker may or may not be able to deduplicate them during an optimization pass (for example, Microsoft's linker does this with OPT:ICF but it is non-compliant to rules about comparison of function pointers). \$\endgroup\$
    – Ben Voigt
    Feb 15 at 21:55
9
\$\begingroup\$

Use the standard library where possible:

stdlib.h defines these functions:

int        abs( int n );
long       labs( long n );
long long llabs( long long n ); // (since C99)

and math.h defines these functions:

float       fabsf( float arg ); // (since C99)
double      fabs( double arg );     
long double fabsl( long double arg ); // (since C99)

// and sqrtf() et cetera.

Consider using them instead of rolling out your own. See also: Pitfalls of function-like macros

Simplify:

float Vector2D_DotProduct(Vector2D a, Vector2D b)
{
#if 0
    float dp = a.x*b.x+a.y*b.y;
    return dp;
#else
    return a.x * b.x + a.y * b.y;
#endif
}

Vector2D Vector2DSubtract(Vector2D a, Vector2D b)
{
#if 0
    Vector2D v;
    v.x = a.x - b.x;
    v.y = a.y - b.y;
    return v;
#else
    return (Vector2D) {.x = a.x - b.x, .y = a.y - b.y};
#endif
}

See: Compound Literals.

\$\endgroup\$
0
5
\$\begingroup\$

Standard library <math.h> provides fabsf(), sqrtf() and hypotf(). No need to reimplement your own.

The usual floating-point type in C is double. Unless you have a strong reason, don't use float for anything!

\$\endgroup\$
3
\$\begingroup\$

Use the type generic tgmath.h. You can then use fabs() for absolute value and sqrt() for square root for either float or double.

fabs() is better than your ABS(n) macro, because the argument to ABS(n) gets expanded twice and could have side-effects or it might contain a function call. For example, x would end up being 3.0 after the following code:

float x=1;
float y=ABS(x++);

The sqrt() function will use hardware (if available) to calculate the square root. It will be many orders of magnitude faster than your implementation.

You might wish to document the API using something like Doxygen.

Doxygen can generate nice HTML documentation from your code. I would generally recommend writing documentation comments in Doxygen style, even if you're not going to build documents. For example

/**
 * Subtract a vector from another vector.
 * \param a First vector
 * \param b Second vector
 * \return \a b subtracted from \a a
 */
Vector2D Vector2DSubtract(Vector2D a, Vector2D b)

Although in this case, the function is so obvious that one might just write something like:

/// Subtract \a a from \a b.
Vector2D Vector2DSubtract(Vector2D a, Vector2D b)

Rename to Vector2f. This name indicates that the underlying type is a float. Alternatively, if you don't plan on ever having Vector2d, you could use just plain Vector2. Or even better, declare it as Vector2f but also declare a typedef Vector2f Vector2;.

Name your functions according to type. Suppose you'll also create a Vector3D. It will be much easier to remember what the functions are (and works better with IDE auto-complete) if you prefix all of your functions with the type. E.g.

Vector2DLength() // Alternatively, Vector2DNorm or Vector2DAbs
Vector2DUnit() // I'd rather have Vector2DNormalize
Vector2DSubtract()
Vector2DDotProduct() // Or maybe just Vector2DDot.

IMO, this looks better in snake case: vector2f_length, vector3f_normalize, etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.