[2/3] git-compat-util: define MIN through compat
diff mbox series

Message ID 20181022053605.81048-3-carenas@gmail.com
State New, archived
Headers show
Series
  • refactor MIN macro
Related show

Commit Message

Carlo Marcelo Arenas Belón Oct. 22, 2018, 5:36 a.m. UTC
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(-)

Comments

Junio C Hamano Oct. 22, 2018, 6:13 a.m. UTC | #1
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;
Carlo Marcelo Arenas Belón Oct. 22, 2018, 6:41 a.m. UTC | #2
I agree with you; dropped

Patch
diff mbox series

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;