Carlo Marcelo Arenas Belónotherwise will warn on platforms where it is already defined (macOS)
convert the internal version from xdiff to a common one from the compatibilty header additionally
threads / rfc / 49632
RFC patch, 3 partsrefactor MIN macro
Subject: [RFC/PATCH 0/3] refactor MIN macro
Carlo Marcelo Arenas BelónCarlo Marcelo Arenas Belón[PATCH 1/3] sha256: avoid redefinition for MIN
it is already defined whenever "sys/param.h" is available
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- sha256/block/sha256.c | 2 ++ 1 file changed, 2 insertions(+)
Show changes to sha256/block/sha256.c +2 −0
diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c index 18350c161a..0d4939cc2c 100644 --- a/sha256/block/sha256.c +++ b/sha256/block/sha256.c @@ -130,7 +130,9 @@ static void blk_SHA256_Transform(blk_SHA256_CTX *ctx, const unsigned char *buf) } } +#ifndef MIN #define MIN(x, y) ((x) < (y) ? (x) : (y)) +#endif void blk_SHA256_Update(blk_SHA256_CTX *ctx, const void *data, size_t len) { const unsigned char *in = data;
-- 2.19.1
Junio C HamanoRe: [PATCH 1/3] sha256: avoid redefinition for MIN
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
Show 6 quoted lines
> it is already defined whenever "sys/param.h" is available > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > sha256/block/sha256.c | 2 ++ > 1 file changed, 2 insertions(+)
It is a no-brainer to say that this is obviously good. I'd rather see this become a part of sha256 topic (perhaps squash it in to eliminate the need to have it as a separate patch), though.
Thanks.
Show 15 quoted lines
>
> diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> index 18350c161a..0d4939cc2c 100644
> --- a/sha256/block/sha256.c
> +++ b/sha256/block/sha256.c
> @@ -130,7 +130,9 @@ static void blk_SHA256_Transform(blk_SHA256_CTX *ctx, const unsigned char *buf)
> }
> }
>
> +#ifndef MIN
> #define MIN(x, y) ((x) < (y) ? (x) : (y))
> +#endif
> void blk_SHA256_Update(blk_SHA256_CTX *ctx, const void *data, size_t len)
> {
> const unsigned char *in = data;Carlo Marcelo Arenas Belón[PATCH 2/3] git-compat-util: define MIN through compat
this macro is commonly defined in system headers (usually <sys/param.h>) but if it is not define it here so it can be used elsewhere
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- git-compat-util.h | 5 +++++ sha256/block/sha256.c | 3 --- 2 files changed, 5 insertions(+), 3 deletions(-)
Show changes to 2 files +5 −3
git-compat-util.h, sha256/block/sha256.c
diff --git a/git-compat-util.h b/git-compat-util.h index 5f2e90932f..7a0b48452b 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -218,6 +218,11 @@ #else #include <stdint.h> #endif + +#ifndef MIN +#define MIN(x, y) ((x) < (y) ? (x) : (y)) +#endif + #ifdef NO_INTPTR_T /* * On I16LP32, ILP32 and LP64 "long" is the safe bet, however diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c index 0d4939cc2c..5fba943c79 100644 --- a/sha256/block/sha256.c +++ b/sha256/block/sha256.c @@ -130,9 +130,6 @@ static void blk_SHA256_Transform(blk_SHA256_CTX *ctx, const unsigned char *buf) } } -#ifndef MIN -#define MIN(x, y) ((x) < (y) ? (x) : (y)) -#endif void blk_SHA256_Update(blk_SHA256_CTX *ctx, const void *data, size_t len) { const unsigned char *in = data;
-- 2.19.1
Junio C HamanoRe: [PATCH 2/3] git-compat-util: define MIN through compat
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
Show 5 quoted lines
> this macro is commonly defined in system headers (usually <sys/param.h>) > but if it is not define it here so it can be used elsewhere > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > ---
I am between "meh" and "moderately negative" on this change.
- Definition of MIN without matching MAX makes me worried about longer-term maintainability. If we were to add MIN() we should also add MAX() to match.
However, "git grep -e '#define MAX(' -e '#define MIN('" tells me
that we do not use these anywhere and Brian's use of a single
MIN() is the only thing that makes this patch interesting. That
is the primary reason why I am very close to "meh" on this.- We need to make sure that this is enough in "git-compat-util.h". Ideally, this should be after all "#include" of the system supplied header files, and before any use of MIN() macro ourselves. I am reasonably sure that the place this patch chose to insert the definition satisfies that criterion within the context of the today's code, but I am unsure if it is even worth having to worry about it in the first place, given how rarely if ever the macro is used. I think Brian's reroll of the sha256 series even gets rid of the use of the macro, as it had only a single place that used the macro anyway.
So...
Show 34 quoted lines
> git-compat-util.h | 5 +++++
> sha256/block/sha256.c | 3 ---
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 5f2e90932f..7a0b48452b 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -218,6 +218,11 @@
> #else
> #include <stdint.h>
> #endif
> +
> +#ifndef MIN
> +#define MIN(x, y) ((x) < (y) ? (x) : (y))
> +#endif
> +
> #ifdef NO_INTPTR_T
> /*
> * On I16LP32, ILP32 and LP64 "long" is the safe bet, however
> diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> index 0d4939cc2c..5fba943c79 100644
> --- a/sha256/block/sha256.c
> +++ b/sha256/block/sha256.c
> @@ -130,9 +130,6 @@ static void blk_SHA256_Transform(blk_SHA256_CTX *ctx, const unsigned char *buf)
> }
> }
>
> -#ifndef MIN
> -#define MIN(x, y) ((x) < (y) ? (x) : (y))
> -#endif
> void blk_SHA256_Update(blk_SHA256_CTX *ctx, const void *data, size_t len)
> {
> const unsigned char *in = data;Carlo Marcelo Arenas Belón[PATCH 3/3] xdiff: use compat's MIN instead
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- xdiff/xdiffi.c | 2 +- xdiff/xemit.c | 6 +++--- xdiff/xhistogram.c | 6 +++--- xdiff/xmacros.h | 4 +--- xdiff/xprepare.c | 2 +- 5 files changed, 9 insertions(+), 11 deletions(-)
Show changes to 5 files +9 −11
xdiff/xdiffi.c, xdiff/xemit.c, xdiff/xhistogram.c, xdiff/xmacros.h, xdiff/xprepare.c
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c index 1f1f4a3c78..0754dc17ed 100644 --- a/xdiff/xdiffi.c +++ b/xdiff/xdiffi.c @@ -204,7 +204,7 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1, fbest = fbest1 = -1; for (d = fmax; d >= fmin; d -= 2) { - i1 = XDL_MIN(kvdf[d], lim1); + i1 = MIN(kvdf[d], lim1); i2 = i1 - d; if (lim2 < i2) i1 = lim2 + d, i2 = lim2; diff --git a/xdiff/xemit.c b/xdiff/xemit.c index 7778dc2b19..43d455b404 100644 --- a/xdiff/xemit.c +++ b/xdiff/xemit.c @@ -217,8 +217,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, post_context_calculation: lctx = xecfg->ctxlen; - lctx = XDL_MIN(lctx, xe->xdf1.nrec - (xche->i1 + xche->chg1)); - lctx = XDL_MIN(lctx, xe->xdf2.nrec - (xche->i2 + xche->chg2)); + lctx = MIN(lctx, xe->xdf1.nrec - (xche->i1 + xche->chg1)); + lctx = MIN(lctx, xe->xdf2.nrec - (xche->i2 + xche->chg2)); e1 = xche->i1 + xche->chg1 + lctx; e2 = xche->i2 + xche->chg2 + lctx; @@ -242,7 +242,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, * its new end. */ if (xche->next) { - long l = XDL_MIN(xche->next->i1, + long l = MIN(xche->next->i1, xe->xdf1.nrec - 1); if (l - xecfg->ctxlen <= e1 || get_func_line(xe, xecfg, NULL, l, e1) < 0) { diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c index ec85f5992b..562cab6d14 100644 --- a/xdiff/xhistogram.c +++ b/xdiff/xhistogram.c @@ -129,7 +129,7 @@ static int scanA(struct histindex *index, int line1, int count1) NEXT_PTR(index, ptr) = rec->ptr; rec->ptr = ptr; /* cap rec->cnt at MAX_CNT */ - rec->cnt = XDL_MIN(MAX_CNT, rec->cnt + 1); + rec->cnt = MIN(MAX_CNT, rec->cnt + 1); LINE_MAP(index, ptr) = rec; goto continue_scan; } @@ -193,14 +193,14 @@ static int try_lcs(struct histindex *index, struct region *lcs, int b_ptr, as--; bs--; if (1 < rc) - rc = XDL_MIN(rc, CNT(index, as)); + rc = MIN(rc, CNT(index, as)); } while (ae < LINE_END(1) && be < LINE_END(2) && CMP(index, 1, ae + 1, 2, be + 1)) { ae++; be++; if (1 < rc) - rc = XDL_MIN(rc, CNT(index, ae)); + rc = MIN(rc, CNT(index, ae)); } if (b_next <= be) diff --git a/xdiff/xmacros.h b/xdiff/xmacros.h index 2809a28ca9..495b9dfac4 100644 --- a/xdiff/xmacros.h +++ b/xdiff/xmacros.h @@ -23,10 +23,8 @@ #if !defined(XMACROS_H) #define XMACROS_H +#include "../git-compat-util.h" - - -#define XDL_MIN(a, b) ((a) < (b) ? (a): (b)) #define XDL_MAX(a, b) ((a) > (b) ? (a): (b)) #define XDL_ABS(v) ((v) >= 0 ? (v): -(v)) #define XDL_ISDIGIT(c) ((c) >= '0' && (c) <= '9') diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c index abeb8fb84e..7436064498 100644 --- a/xdiff/xprepare.c +++ b/xdiff/xprepare.c @@ -451,7 +451,7 @@ static int xdl_trim_ends(xdfile_t *xdf1, xdfile_t *xdf2) { recs1 = xdf1->recs; recs2 = xdf2->recs; - for (i = 0, lim = XDL_MIN(xdf1->nrec, xdf2->nrec); i < lim; + for (i = 0, lim = MIN(xdf1->nrec, xdf2->nrec); i < lim; i++, recs1++, recs2++) if ((*recs1)->ha != (*recs2)->ha) break;
-- 2.19.1
Junio C HamanoRe: [PATCH 3/3] xdiff: use compat's MIN instead
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
Show 8 quoted lines
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > xdiff/xdiffi.c | 2 +- > xdiff/xemit.c | 6 +++--- > xdiff/xhistogram.c | 6 +++--- > xdiff/xmacros.h | 4 +--- > xdiff/xprepare.c | 2 +- > 5 files changed, 9 insertions(+), 11 deletions(-)
As this is a borrowed code, I'd rather not do this, especially do so only for MIN so that people who update this need to be aware that they can say MIN() but still have to say XDL_MAX().
> - > -#define XDL_MIN(a, b) ((a) < (b) ? (a): (b)) > #define XDL_MAX(a, b) ((a) > (b) ? (a): (b))