From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 09 Oct 2024 16:13:33 GMT
Subject: Re: [PATCH] mimgw: remove Compiler Warnings
Message-ID: <4530b7cc-3f91-4009-977e-97519a5a9f85@gmail.com>
In-Reply-To: <20241009103541.2887-1-soekkle@freenet.de>

Hi Sören

On 09/10/2024 11:35, Sören Krecker wrote:
> Remove some complier warnings from msvc in compat/mingw.c for value truncation from 64 bit to 32 bit intigers.

Thanks for working on this, it is a useful improvement. It would be 
helpful to explain the choice of signed/unsigned type in each case to 
help reviewers check that the conversion is correct. I've looked through 
the code and there are a couple I'm not sure about. I'd also echo 
Torsten's code formatting comments.

> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>   compat/compiler.h               |  4 ++--
>   compat/mingw.c                  | 26 ++++++++++++++++----------
>   compat/vcbuild/include/unistd.h |  7 +++++++
>   3 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/compat/compiler.h b/compat/compiler.h
> index e9ad9db84f..e12e426404 100644
> --- a/compat/compiler.h
> +++ b/compat/compiler.h
> @@ -9,7 +9,7 @@
>   
>   static inline void get_compiler_info(struct strbuf *info)
>   {
> -	int len = info->len;
> +	size_t len = info->len;
>   #ifdef __clang__
>   	strbuf_addf(info, "clang: %s\n", __clang_version__);
>   #elif defined(__GNUC__)
> @@ -27,7 +27,7 @@ static inline void get_compiler_info(struct strbuf *info)
>   
>   static inline void get_libc_info(struct strbuf *info)
>   {
> -	int len = info->len;
> +	size_t len = info->len;
>   
>   #ifdef __GLIBC__
>   	strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version());

These two look straight forward - we save info->len at the start of the 
function and see if it has changed at the end.

> diff --git a/compat/mingw.c b/compat/mingw.c
> index 0e851ecae2..dca0816267 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -782,7 +782,7 @@ static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
>    */
>   static int has_valid_directory_prefix(wchar_t *wfilename)
>   {
> -	int n = wcslen(wfilename);
> +	ssize_t n = wcslen(wfilename);

This is ssize_t because n maybe negative as seen in the context below - good

>   
>   	while (n > 0) {
>   		wchar_t c = wfilename[--n];
> @@ -891,7 +891,7 @@ static int do_lstat(int follow, const char *file_name, struct stat *buf)
>    */
>   static int do_stat_internal(int follow, const char *file_name, struct stat *buf)
>   {
> -	int namelen;
> +	ssize_t namelen;

Looking at this function I can't see why this is ssize_t rather than size_t

> @@ -1274,7 +1274,8 @@ static const char *parse_interpreter(const char *cmd)
>   {
>   	static char buf[100];
>   	char *p, *opt;
> -	int n, fd;
> +	ssize_t n;

This is ssize_t because we assign the return value of read() to n which 
maybe negative - good

> @@ -1339,7 +1340,7 @@ static char *path_lookup(const char *cmd, int exe_only)
>   {
>   	const char *path;
>   	char *prog = NULL;
> -	int len = strlen(cmd);
> +	size_t len = strlen(cmd);

This looks good we're holding the length of a string

>   	int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe");
>   
>   	if (strpbrk(cmd, "/\\"))
> @@ -1956,7 +1957,7 @@ char *mingw_getenv(const char *name)
>   #define GETENV_MAX_RETAIN 64
>   	static char *values[GETENV_MAX_RETAIN];
>   	static int value_counter;
> -	int len_key, len_value;
> +	size_t len_key, len_value;

This looks good too, they're holding the length of a string

> @@ -2001,7 +2005,7 @@ char *mingw_getenv(const char *name)
>   
>   int mingw_putenv(const char *namevalue)
>   {
> -	int size;
> +	size_t size;

This looks correct - another string length

> @@ -3085,7 +3090,8 @@ static void maybe_redirect_std_handles(void)
>    */
>   int wmain(int argc, const wchar_t **wargv)
>   {
> -	int i, maxlen, exit_status;
> +	int exit_status;
> +	size_t i, maxlen;

"i" loops over 0..argc so I think we want to keep it as an int. maxlen 
is a string length so should be size_t.

Best Wishes

Phillip

>   	char *buffer, **save;
>   	const char **argv;
>   
> diff --git a/compat/vcbuild/include/unistd.h b/compat/vcbuild/include/unistd.h
> index 3a959d124c..ab3dc06709 100644
> --- a/compat/vcbuild/include/unistd.h
> +++ b/compat/vcbuild/include/unistd.h
> @@ -13,8 +13,15 @@ typedef _mode_t	mode_t;
>   #endif	/* Not _MODE_T_ */
>   
>   #ifndef _SSIZE_T_
> +#ifdef _WIN64
>   #define _SSIZE_T_
> +typedef __int64 _ssize_t;
> +#pragma message("Compiling on Win64")
> +#else
>   typedef long _ssize_t;
> +#endif // _AMD64
> +
> +
>   
>   #ifndef	_OFF_T_
>   #define	_OFF_T_
> 
> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2