| Message ID | 20181022053605.81048-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refactor MIN macro | expand |
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;
I agree with you; dropped
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;
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(-)