Can you introduce a corresponding MAX macro, while you are at it?

Can't we use a static inline for this?

Using a static inline would make this function type strict, using a macro will let the same macro be used for both signed and unsigned integers as well as floats.

Can you introduce a corresponding MAX macro, while you are at it?

I thought about it, but then I decided against introducing something that has no use case currently. It's a small change to introduce whenever we need it.

ACK.

For reference, the following C files use some form of min (searched with git grep -i "\<_\?min\>(")`:

• sys/chardev_thread.c
• sys/crypto/modes/ccm.c
• sys/net/gnrc/network_layer/sixlowpan/frag/gnrc_sixlowpan_frag.c
• tests/pip/main.c

Good catch. Updated the PR.

Care to add a unittests to tests/unittests/tests-core/ for the macro?

Will do.

NACK. what happened to "we try to avoid macros"?

Actually, this PR does not change the number of macros. Of course, we can make an inline function for every comparable data type, but this one of the few cases where even a macro-allergic person like me prefer it.

The name MIN is bound to cause name clashes.

I'd prefer an implementation that does not evaluate the parameters twice:

#define MIN(A, B) ({ \
    __typeof__(A) __a = (A); \
    __typeof__(B) __b = (B); \
    (__a < __b) ? __a : __b; \
})

Actually I think int16_t min_i16(int16_t, int16_t), uint16_t min_u64(uint64_t, int64_t), ... wouldn't be too bad either:

for m in ('min<', 'max>'):
    for p in ('u',''):
        for i in range(4):
            print('''
static inline {p}int{j}_t {m}_{q}{j}({p}int{j}_t a, {p}int{j}_t j)
{{
    return (a {o} b) ? a : b;
}}'''.format(m=m[:-1], o=m[-1], p=p, q=p or 'i', j=8<<i))

Brrrr. Here I'm with @kaspar030 (and the coding conventions: "Do NOT use global macros defining more than one line of code. Use inline functions instead."). That looks awful.

So ... you don't like

static inline uint8_t min_u8(uint8_t a, uint8_t j)
{
    return (a < b) ? a : b;
}

static inline uint16_t min_u16(uint16_t a, uint16_t j)
{
    return (a < b) ? a : b;
}

static inline uint32_t min_u32(uint32_t a, uint32_t j)
{
    return (a < b) ? a : b;
}

static inline uint64_t min_u64(uint64_t a, uint64_t j)
{
    return (a < b) ? a : b;
}

static inline int8_t min_i8(int8_t a, int8_t j)
{
    return (a < b) ? a : b;
}

static inline int16_t min_i16(int16_t a, int16_t j)
{
    return (a < b) ? a : b;
}

static inline int32_t min_i32(int32_t a, int32_t j)
{
    return (a < b) ? a : b;
}

static inline int64_t min_i64(int64_t a, int64_t j)
{
    return (a < b) ? a : b;
}

static inline uint8_t max_u8(uint8_t a, uint8_t j)
{
    return (a > b) ? a : b;
}

static inline uint16_t max_u16(uint16_t a, uint16_t j)
{
    return (a > b) ? a : b;
}

static inline uint32_t max_u32(uint32_t a, uint32_t j)
{
    return (a > b) ? a : b;
}

static inline uint64_t max_u64(uint64_t a, uint64_t j)
{
    return (a > b) ? a : b;
}

static inline int8_t max_i8(int8_t a, int8_t j)
{
    return (a > b) ? a : b;
}

static inline int16_t max_i16(int16_t a, int16_t j)
{
    return (a > b) ? a : b;
}

static inline int32_t max_i32(int32_t a, int32_t j)
{
    return (a > b) ? a : b;
}

static inline int64_t max_i64(int64_t a, int64_t j)
{
    return (a > b) ? a : b;
}

neither? :)

If defined static inline in a header, couldn't we go with the 64bit versions only? Within a single compilation unit, the compiler would use optimized instructions anyways.

@kaspar030 should love it, but I would actually rather prefer to replace all calls of MIN() by ((a > b) ? a : b) literally over one of these solutions.

Wow, five core maintainers disscussing a min/max macro PR with already finished documentation an unit-tests. I think we should use this as an opportunity to learn how to come to a fruitful result that everyone agrees with, if possible.

I propose being a little more methodical on how to choose one of the possible solutions.

-. let's get "personal preferences" out of the way by backing them up with technical arguments
e.g. "I prefer X over Y because X uses less lines of code"

• let's collect the pro's and con's of each solution in an objective manner
  e.g.

"Solution A:
pro:

• is type-agnostic

con:

• it's a macro, so it possibly evaluates arguments twice, and might behave differently depending on which parameter types are being used in which context

Solution B:

pro:

• deterministic behaviour

con:

• needs per-type definitions
  "

Solution A:

• pro:
  • saves 25 lines of code

Solution B:

• con:
  • introduces hundreds new lines of code where most of them are probably never used

With unsigned it's only 8 bytes bigger.

Just tried, I get 2 bytes less e for "unsigned". How do you test?

I got this:

[kaspar@booze hello-world (wtimer)]$ cat min_test.c 
#include "minmax.h"
unsigned min_test(unsigned a, unsigned b) {
    return MIN(a,b);
}
[kaspar@booze hello-world (wtimer)]$ 

and replace MIN with min, defined as

static inline unsigned min(unsigned a, unsigned b) {
   return a<b?a:b;
}

introduces hundreds new lines of code where most of them are probably never used

Could you elaborate?

See #3691 (comment) (probably not "hundreds", but "tens").

All current uses of min are using unsigned or size_t, correct? (chardev doesn't, but it should, and it is about to get removed from the code).

Just tried, I get 2 bytes less e for "unsigned". How do you test?

https://github.com/OlegHahm/miniature-dangerzone/tree/master/min-test

The random stuff is just there to prevent the compiler from optimizing.

All current uses of min are using unsigned or size_t, correct?

The cipher stuff seems to use uint8_t.

Gotta run. Will be offline for about 1 hour.

Did some more testing.

While using the correct typed version of min/max as static inline (like @Kijewski's proposal) always leads to smallest code, using a macro "most of the times" produces as-small code, across multiple platforms (avr8, msp430, cortexM0+3), with just some exceptions.
Using only, e.g., signed or unsigned static inline functions for all data types is least efficient.

So do we use the macro, document it's drawbacks, and accept a little inefficiency in corner cases, but have the convinience of a single line of macro code, or do we go with 16 static inline functions (64 lines of mostly identical code), all containing the same tertiary operator, forcing the user to choose the correctly typed one, but ensuring maximum efficiency while ruling out possible side effects caused by involving the preprocessor?

@kaspar030, thanks for the thorough analysis. I would still prefer the macro solution for simplicity.

I prefer my convoluted macro solution but I guess you two don't. :)

I actually prefer the typeof based macro over the simple one. It's even ISO C, right?

As far as I can tell, it's not ANSI C and I will not let the coding conventions be bent here (again). One-line macro, inline functions, or literal replacement - these are the options.

So, then let's go with the static inlines...

Stuck in a loop.

MIN() is perfectly fine IMHO, defining 20+ inline function, that would require casting do not help anybody (there are even types missing, what's with mixed comparisons e.g.). On the other hand I agree, that we should try to avoid double-evaluation of the parameters. But that can be done by fixing it at the caller as well.

IMO, having min as typed inlines is more verbose and hence the code becomes clearer about the context it is used in. Besides, why would you need casting? You can use the inline with the bigger width of the two parameters. The compiler won't complain if you put an uint8_t into a function which expects an uint32_t

So reduces the types of functions needed to

static inline min(int a, int b)
{
    return (a < b) ? a : b;
}

static inline minu(unsigned int a, unsigned int b)
{
    return (a < b) ? a : b;
}

static inline minll(long long int a, long long int b)
{
    return (a < b) ? a : b;
}

static inline minllu(long long unsigned int a, long long unsigned int b)
{
    return (a < b) ? a : b;
}

Or am I wrong?

In theory, but in practice, this enlarges the code...

Why?

For cortex platforms it could be that the instruction width selection fails to choose the optimal one. So that e.g. a 32-bit instruction instead of a 16-bit instruction is used for a comparison of two uint8_ts. But I didn't look into the machine code here, just speculating.

I have obviously to little of an opinion about that => Reassign. Call for a second (formality-check) ACK if needed ;-)

Actually, I'm about to close this PR since we're apparently failing to conclude on something.

I really have no strong opinion on this anymore. But looking at the diff, I see several calls to the min macro with a subtraction as a parameter. I am not sure about how smart the compiler is with regards to caching the result in a register, it probably depends on the use case, but this again is IMO a showcase for the weakness of macros: double evaluation. I am fine with having a min macro defined somewhere. It's still up to the developer if she or he wants to use the macro or not.

Closed because of lack of consensus.

Poor, for a PR where someone (me) proposed to use it to actually find a solution.