[RFC/PATCH 0/3] refactor MIN macro

[RFC/PATCH 0/3] refactor MIN macro

[Carlo Marcelo Arenas Belón]

otherwise 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

[PATCH 1/3] sha256: avoid redefinition for MIN

[Carlo Marcelo Arenas Belón]

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(+)

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

Re: [PATCH 1/3] sha256: avoid redefinition for MIN

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 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.

>
> 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;

[PATCH 2/3] git-compat-util: define MIN through compat

[Carlo Marcelo Arenas Belón]

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(-)

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

Re: [PATCH 2/3] git-compat-util: define MIN through compat

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 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...

>  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;

Re: [PATCH 2/3] git-compat-util: define MIN through compat

[Carlo Arenas]

I agree with you; dropped

[PATCH 3/3] xdiff: use compat's MIN instead

[Carlo Marcelo Arenas Belón]

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(-)

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

Re: [PATCH 3/3] xdiff: use compat's MIN instead

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 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))