diff mbox series

mimgw: remove Compiler Warnings

Message ID 20241009103541.2887-1-soekkle@freenet.de (mailing list archive)
State Superseded
Headers show
Series mimgw: remove Compiler Warnings | expand

Commit Message

Sören Krecker Oct. 9, 2024, 10:35 a.m. UTC 
Remove some complier warnings from msvc in compat/mingw.c for value truncation from 64 bit to 32 bit intigers.

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


base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2

Comments

Torsten Bögershausen Oct. 9, 2024, 3:26 p.m. UTC | #1 
On Wed, Oct 09, 2024 at 12:35:41PM +0200, Sören Krecker wrote:

Thanks for the patch. All looks sensible - 2 comments inline.

> Remove some complier warnings from msvc in compat/mingw.c for value truncation from 64 bit to 32 bit intigers.
small typo: compiler
>
> 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());
> 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);
>
>  	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;
>  	char alt_name[PATH_MAX];
>
>  	if (!do_lstat(follow, file_name, buf))
> @@ -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;
> +	int fd;
>
>  	/* don't even try a .exe */
>  	n = strlen(cmd);
> @@ -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);
>  	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;
>  	wchar_t *w_key;
>  	char *value;
>  	wchar_t w_value[32768];
> @@ -1968,7 +1969,9 @@ char *mingw_getenv(const char *name)
>  	/* We cannot use xcalloc() here because that uses getenv() itself */
>  	w_key = calloc(len_key, sizeof(wchar_t));
>  	if (!w_key)
> -		die("Out of memory, (tried to allocate %u wchar_t's)", len_key);
> +		die("Out of memory, (tried to allocate %"

Is there a reason to split the line like this ?

> +			    PRIuMAX" wchar_t's)",
> +		    (uintmax_t)len_key);
>  	xutftowcs(w_key, name, len_key);
>  	/* GetEnvironmentVariableW() only sets the last error upon failure */
>  	SetLastError(ERROR_SUCCESS);
> @@ -1983,7 +1986,8 @@ char *mingw_getenv(const char *name)
>  	/* We cannot use xcalloc() here because that uses getenv() itself */
>  	value = calloc(len_value, sizeof(char));
>  	if (!value)
> -		die("Out of memory, (tried to allocate %u bytes)", len_value);
> +	    die("Out of memory, (tried to allocate %" PRIuMAX " bytes)",
> +	    (uintmax_t)len_value);

Indentation should be a TAB, not 4 spaces. And why this line-split ?

>  	xwcstoutf(value, w_value, len_value);
>
>  	/*
> @@ -2001,7 +2005,7 @@ char *mingw_getenv(const char *name)
>
>  int mingw_putenv(const char *namevalue)
>  {
> -	int size;
> +	size_t size;
>  	wchar_t *wide, *equal;
>  	BOOL result;
>
> @@ -2011,7 +2015,8 @@ int mingw_putenv(const char *namevalue)
>  	size = strlen(namevalue) * 2 + 1;
>  	wide = calloc(size, sizeof(wchar_t));
>  	if (!wide)
> -		die("Out of memory, (tried to allocate %u wchar_t's)", size);
> +		die("Out of memory, (tried to allocate %" PRIuMAX " wchar_t's)",
> +		    (uintmax_t)size);
>  	xutftowcs(wide, namevalue, size);
>  	equal = wcschr(wide, L'=');
>  	if (!equal)
> @@ -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;
>  	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

Is this needed, or is it a rest from a trial to use _ssize_t instead of ssize_t ?

> +
> +
>
>  #ifndef	_OFF_T_
>  #define	_OFF_T_
>
> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
> --
> 2.39.5
>
>
Phillip Wood Oct. 9, 2024, 4:13 p.m. UTC | #2 
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
Sören Krecker Oct. 9, 2024, 5:13 p.m. UTC | #3 
Hi Torsten, Hi Phillip,

I have tried to implement your comments. I add som comments after the verable to explain what there contains.
I try to do less a possiable changes, the _ssize_t was there before and I change the type on win 64.

Best regards,

Sören Krecker

Sören Krecker (1):
  mimgw: remove Compiler Warnings

 compat/compiler.h               |  4 ++--
 compat/mingw.c                  | 25 +++++++++++++++----------
 compat/vcbuild/include/unistd.h |  4 ++++
 3 files changed, 21 insertions(+), 12 deletions(-)


base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
diff mbox series

Patch

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());
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);
 
 	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;
 	char alt_name[PATH_MAX];
 
 	if (!do_lstat(follow, file_name, buf))
@@ -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;
+	int fd;
 
 	/* don't even try a .exe */
 	n = strlen(cmd);
@@ -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);
 	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;
 	wchar_t *w_key;
 	char *value;
 	wchar_t w_value[32768];
@@ -1968,7 +1969,9 @@  char *mingw_getenv(const char *name)
 	/* We cannot use xcalloc() here because that uses getenv() itself */
 	w_key = calloc(len_key, sizeof(wchar_t));
 	if (!w_key)
-		die("Out of memory, (tried to allocate %u wchar_t's)", len_key);
+		die("Out of memory, (tried to allocate %"
+			    PRIuMAX" wchar_t's)",
+		    (uintmax_t)len_key);
 	xutftowcs(w_key, name, len_key);
 	/* GetEnvironmentVariableW() only sets the last error upon failure */
 	SetLastError(ERROR_SUCCESS);
@@ -1983,7 +1986,8 @@  char *mingw_getenv(const char *name)
 	/* We cannot use xcalloc() here because that uses getenv() itself */
 	value = calloc(len_value, sizeof(char));
 	if (!value)
-		die("Out of memory, (tried to allocate %u bytes)", len_value);
+	    die("Out of memory, (tried to allocate %" PRIuMAX " bytes)",
+	    (uintmax_t)len_value);
 	xwcstoutf(value, w_value, len_value);
 
 	/*
@@ -2001,7 +2005,7 @@  char *mingw_getenv(const char *name)
 
 int mingw_putenv(const char *namevalue)
 {
-	int size;
+	size_t size;
 	wchar_t *wide, *equal;
 	BOOL result;
 
@@ -2011,7 +2015,8 @@  int mingw_putenv(const char *namevalue)
 	size = strlen(namevalue) * 2 + 1;
 	wide = calloc(size, sizeof(wchar_t));
 	if (!wide)
-		die("Out of memory, (tried to allocate %u wchar_t's)", size);
+		die("Out of memory, (tried to allocate %" PRIuMAX " wchar_t's)",
+		    (uintmax_t)size);
 	xutftowcs(wide, namevalue, size);
 	equal = wcschr(wide, L'=');
 	if (!equal)
@@ -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;
 	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_