11
\$\begingroup\$

I'm writing a toy vector library in C to learn a bit about the language. The excerpt below shows my vector structure and two functions:

  • vec2_magnitude calculates the magnitude (length) of a vector
  • vec2_unit uses the magnitude to calculate the unit vector
#include <math.h>

// A fixed-length vector w/ 2 elements.
typedef struct
{
        double a;
        double b;

} vec2_t;

// Compute the magnitude of a vec2_t.
double vec2_magnitude(const vec2_t *vec)
{
        double a2 = vec->a * vec->a;
        double b2 = vec->b * vec->b;
        return sqrt(a2 + b2);
}

// Compute the unit vector of a vec2_t.
void vec2_unit(const vec2_t *vec, vec2_t *unit)
{
        double mag = vec2_magnitude(vec);
        unit->a = (vec->a / mag);
        unit->b = (vec->b / mag);
}

Currently, vec2_unit takes two vector arguments. It performs the calculation based on the first argument, then stores its result in the second argument. I wrote the function this way so the original vector wouldn't be affected (in case it is still needed).

That being said, I'd like a way to calculate the unit vector in-place, since you may not always need the old vector (in such a situation, an in-place calculation is more convenient). I was originally going to write two functions, but noticed a clever way to achieve both functionalities using only the function shown above.

To operate non-destructively, vec2_unit can be called like this:

vec2_t orig_vector = {1, 1};
vec2_t unit_vector;
vec2_unit(orig_vector, unit_vector);

To operate in-place, vec2_unit can be called like this:

vec2_t orig_vector = {1, 1};
vec2_unit(orig_vector, orig_vector);

I like this solution, and it seems to work. I also understand that C is a language where many things aren't "safe", and just because you can do something doesn't mean you should. Are there any reasons something like this shouldn't be done? Are there other examples of common C functions that work in a similar way? Is there a better way to accomplish what I'm trying to do?

I appreciate all help. Feel free to comment on anything else you see as well. Thanks!

\$\endgroup\$
0

5 Answers 5

9
\$\begingroup\$
  • It is custom/industry de facto standard to place the parameter to be changed first, as done in memcpy, strcpy etc (this tradition goes all the way back to assembler).

    So you should do void vec2_unit (vec2_t* dst, const vec2_t* src);.

  • Regarding in-place or not, it is mostly a matter of style in this case.

    Functions/APIs working on "immutable" objects (objects not modified by the function) are often considered better style when that option is available, since that minimizes the chance of caller-side bugs.

    The discussion about in-place vs immutable mostly makes sense for larger data types, where taking a copy of the data object is regarded as costly. A struct with 2 double isn't really that heavy to copy(note 1), so you could design this API with an immutable object interface. You could even get away with passing the structs to/from the functions by value, which is otherwise normally frowned upon.

    Note: if these functions will reside in the same translation unit as the caller code (which doesn't seem likely here?), the whole discussion about performance is pointless since they will be inlined anyway in that case.

  • If you go for the immutable version, it could be written as

    void vec2_unit (vec2_t* restrict dst, const vec2_t* restrict src);

    This tells the compiler and the caller both that these two objects shall not be the same one. That will in turn increase performance, but it will not allow you to pass the same object as both source and destination.


(note 1) We can assume that systems using double are able to copy it in a few instructions. Systems where copying a double would be lots of work, such as small microcontroller systems, shouldn't be using double (or float) in the first place, since they lack a FPU. I would safely assume that any program using floating point is meant to run on a Cortex M3 or bigger.

\$\endgroup\$
2
  • \$\begingroup\$ vec2_unit (vec2_t* restrict dst, const vec2_t* restrict src); also implies there is no overlap either, not only dst != src. \$\endgroup\$ Dec 12, 2019 at 19:07
  • \$\begingroup\$ "You could even get away with passing the structs to/from the functions by value, ..." --> Agree, especially when the objects represent a "number" as most <math.h> functions/parameters use this model. \$\endgroup\$ Dec 12, 2019 at 20:01
7
\$\begingroup\$

Instead of summing squares manually and passing to sqrt(), we should be using the hypot() function. That's functionally similar, but has better guarantees of accuracy and correctness (particularly when faced with unusual inputs, such as subnormals).

double vec2_magnitude(const vec2_t *vec)
{
    return hypot(vec->a, vec->b);
}
\$\endgroup\$
4
\$\begingroup\$

Personaly I would put the non constant argument first:

I would also check for the first argument for being null too.

And you can define a shortcut function for passing the same argument twice.

double vec2_magnitude(const vec2_t *vec)
{
        if (vec == NULL) return NaN;
        double a2 = vec->a * vec->a;
        double b2 = vec->b * vec->b;
        return sqrt(a2 + b2);
}

int vec2_unit(vec2_t * unit, const vec2_t * vec)
{
        if (unit == NULL) return -1;
        double mag = vec2_magnitude(vec);
        if (mag == 0.0 || isnan(mag)) return -1;
        unit->a = (vec->a / mag);
        unit->b = (vec->b / mag);
        return 0;
}

int vec2_norm(vec2_t * vec)
{
        return vec2_unit(vec, vec);
}

Further, division by zero is forbidden and so I check for that as well...

\$\endgroup\$
0
4
\$\begingroup\$

It is not uncommon for C API functions to simply branch if NULL pointers are received. Consider the following:

void vec2_unit(vec2_t *vec, vec2_t* unit)
{
        double mag = vec2_magnitude(vec);

        if(unit == NULL){
            vec->a /= mag;
            vec->b /= mag;
            return;
        }

        unit->a = vec->a / mag;
        unit->b = vec->b / mag;
}

Here you update the unit vector only if it is supplied. Otherwise vec is updated. If you wanted to be more verbose, you could also consider:

// You can name this macro whatever you'd like
#define PERFORM_OP_INPLACE NULL

int main(){
    vec2_t vec = {1.0, 1.0};

    vec2_unit(&vec, PERFORM_OP_INPLACE);
}
\$\endgroup\$
3
  • \$\begingroup\$ Instead of if(unit == NULL){ vec->a /= mag; vec->b /= mag; return; }, coudl use if(unit == NULL){ vec = unit; } \$\endgroup\$ Dec 12, 2019 at 19:35
  • \$\begingroup\$ @chux-ReinstateMonica True, but it would actually be if(unit == NULL){ unit = vec; } and the you can edit unit. :) \$\endgroup\$
    – Wil
    Dec 12, 2019 at 22:27
  • \$\begingroup\$ .backwards that had I Yes \$\endgroup\$ Dec 12, 2019 at 22:29
3
\$\begingroup\$

In general you can "remove" const legally if the original variable wasn't declared const, ie.

int a;
const int c;

void foo(const int *x)
{
    int *p = (int *)x;
    p++;
}

foo(&a); // legal
foo(&c); // illegal

The rationale is that const-variables are often declared in read-only memory segments. As far as I know, the compiler is not allowed more than that (const doesn't have much impact in C, Ritchie even didn't want it included in C). Additionally the compiler cannot assume that the memory areas do not overlap (because they aren't declared as restrict), so this code should be completely legal and even safe as it is only illegal if the argument was defined to be const:

const vec2_t v;
vec2_unit(&v, &v);

But then it should warn about the second argument discarding const.

Additional note: vec2_t is a reserved type by POSIX, as are all types with suffix _t, you are encouraged to use a different type name.

\$\endgroup\$
1
  • \$\begingroup\$ @chux-ReinstateMonica Ah, I've missed the &, thanks for pointing out! \$\endgroup\$
    – ljrk
    Dec 13, 2019 at 9:25

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.