3
\$\begingroup\$

I've come up with some simple implementation for algorithms in C and more specifically for std::accumulate alternate since I need something like it recently. Can you provide some review on the approach?

#include <stdio.h>
#include <string.h>


#define ARRSIZE(X) (sizeof(X) / sizeof(X[0]))


#define ACCUMULATE(RET, FIRST, LAST, INIT, SUM_CB)      \
    do {                                                \
        RET = INIT;                                     \
        while (FIRST != LAST){                          \
            RET = SUM_CB(RET, *FIRST);                  \
            FIRST++;                                    \
        }                                               \
    } while(0);

/**
 * example usage
 * sum integers and sum strings by some delimiter
*/
struct str_t
{
    char data[512];
};



struct str_t sumStr(struct str_t a, struct str_t b)
{
    struct str_t ret = {0};
    strcat(ret.data, a.data);
    strcat(ret.data, "--");
    strcat(ret.data, b.data);
    return ret;
}


struct A { int a, b, c; };

struct A sumA(struct A a, struct A b) {
    struct A ret ;
    ret.a = a.a + b.a;
    ret.b = a.b + b.b;
    ret.c = a.c + b.c;
    return ret;
}


int main()
{
    unsigned long i;

    struct A sumvec[10];
    struct str_t sumstr[10];
    memset(sumstr, 0, sizeof(sumstr));

    for(i=0; i < ARRSIZE(sumvec); i++) {
        sumvec[i].a = i;
        sumvec[i].b = i+1;
        sumvec[i].c = i+2;
        snprintf(sumstr[i].data, sizeof(sumstr[i].data), "%d", i);
    }

    for(i=0; i < ARRSIZE(sumvec); i++) {
        printf("[%d][%d][%d]\r\n", sumvec[i].a ,sumvec[i].b,sumvec[i].c);
    }


    struct A ret = {sumvec[0].a, sumvec[0].b, sumvec[0].c};
    struct A* pBegin = &sumvec[1];
    struct A* pEnd = &sumvec[ARRSIZE(sumvec)];

    ACCUMULATE(ret, pBegin, pEnd,  ret, sumA)

    printf("SUM: [%d][%d][%d]\r\n", ret.a, ret.b, ret.c);


    struct str_t retstr;
    strcat(retstr.data, sumstr[0].data);

    struct str_t *pBegs = &sumstr[1];
    struct str_t *pEnds = &sumstr[ARRSIZE(sumstr)];

    ACCUMULATE(retstr, pBegs, pEnds, retstr, sumStr);

    printf("SUM STR: [%s]\r\n", retstr.data);
    return 0;
}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

In addition to @Toby Speight fine answer.

Can you provide some review on the approach?

I found the macro ACCUMULATE(), at best, slightly useful. I did not see it adding clarity. Perhaps it would be a useful benefit in a larger code usage.

A particular issue with ACCUMULATE(a, b,c, d, e) is the 9 usages of the 5 arguments - potentially incurring re-use problems.

At a minimum ACCUMULATE(), deserve some comments detailing its usage, simpler sample usage and limitations. Do not assume C users are familiar with std::accumulate from another language.


I realize this is demonstrative code. Yet ...

Bug

Attempting to concatenate to an uninitialized string. Use strcpy().

struct str_t retstr;
// strcat(retstr.data, sumstr[0].data);
strcpy(retstr.data, sumstr[0].data);

Walk a string once, not 3 times

As strlen(ret.data) increases, strcat(ret.data, ... becomes increasingly expensive.

//strcat(ret.data, a.data);
//strcat(ret.data, "--");
//strcat(ret.data, b.data);
sprintf(ret.data, "%s--%s", a.data, b.data);

Robust would consider using snprintf().

Simplify initialization

// struct str_t sumstr[10];
// memset(sumstr, 0, sizeof(sumstr));
struct str_t sumstr[10] = { 0 };

// struct A ret = {sumvec[0].a, sumvec[0].b, sumvec[0].c};
struct A ret = sumvec[0];

\r\n vs. \n"

stdout is normally in text mode and will translates '\n' into the end-of-line- encoding best for that system. Recommend '\n'.

// printf("[%d][%d][%d]\r\n", sumvec[i].a ,sumvec[i].b,sumvec[i].c);
printf("[%d][%d][%d]\n", sumvec[i].a ,sumvec[i].b,sumvec[i].c);

Minor: Unclear mixed use of types unsigned long and int

struct A { int a, b, c; };

    unsigned long i;
    sumvec[i].a = i;
    sumvec[i].b = i+1;
    sumvec[i].c = i+2;

Why is i unsigned long and not int to match the sstruct members or size_t to match the range for indexing?

\$\endgroup\$
2
  • \$\begingroup\$ Thanks. Good point but I didn’t focus on example usage just a demo of the macro, but will fix it. \$\endgroup\$ Commented Dec 20, 2020 at 10:11
  • \$\begingroup\$ Agh! I had the \r in my mind, then forgot it again whilst writing my answer! It always helps to have more than one of us reviewing... \$\endgroup\$ Commented Dec 21, 2020 at 15:55
3
\$\begingroup\$

When using the do { ... } while (0) idiom for macros, we don't generally include the terminating semicolon in the definition. That's so we can use it like a function call:

if (condition)
    ACCUMULATE(result, a, b, 0, add);
else
    ACCUMULATE(result, a, c, 10, add);

There's a typo in the ARRSIZE() macro: where we protect X from misbinding - should be (X)[0], not (X[0]):

#define ARRSIZE(X) (sizeof (X) / sizeof (X)[0])

Also, be careful with this macro when its argument isn't a whole, completely defined array. In particular, it will be wrong when the argument is a variable-length (dynamically-allocated) array, or when the argument is a pointer (perhaps the array decayed to pointer - that can easily catch you out!).

In the sample usage:

  • Identifiers ending in _t are reserved for future use by the standard library, so str_t should be renamed.
  • It's a shame, after being so careful to use snprintf() avoid overflow, that we we then use unchecked strcat();strcat();strcat(); in sumStr() - why not snprintf() there, too?
  • A lot of needless parens around the arguments to sizeof. Just write sizeof sumstr instead of sizeof(sumstr), for example.
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You may also want to mention that the ARRSIZE macro often confuses people when it decays to a pointer and that it doesn't work with dynamically sized arrays. \$\endgroup\$
    – Edward
    Commented Dec 19, 2020 at 21:26
  • \$\begingroup\$ Thanks @Edward - edited. \$\endgroup\$ Commented Dec 21, 2020 at 15:53

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.