| Message ID | 20241009171342.2354-2-soekkle@freenet.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] mimgw: remove Compiler Warnings | expand |
Sören Krecker <soekkle@freenet.de> writes: > Remove some compiler warnings from msvc in compat/mingw.c for value truncation from 64 bit to 32 bit intigers. An overly long line? > diff --git a/compat/mingw.c b/compat/mingw.c > index 0e851ecae2..5293f4cdae 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); /*can become negative*/ Aside from the malformed comment ("/* can become negative */" with spaces would have been OK), I am not sure where it can become negative, unless wcslen() is allowed to return a negative value to signal some kind of error (in which case, the comment should say that), which is not the case. The loop body in the post-context of this hunk looks like > while (n > 0) { > wchar_t c = wfilename[--n]; ... 'n' is not written anywhere else in this loop ... } so an 'n' that is not negative before entering the loop can never become negative by what the loop body does. > @@ -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; > + size_t namelen; /* contains length of a string*/ Indeed, this receives the return value of strlen(). I am not sure if this comment is necessary, though. Just like you omitted any comment on size_t variables that receives .len in a strbuf, its correctness is rather obvious. > @@ -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; /* read() can return negativ values */ The word is "negative". But 'n' is also used to receive the result of strlen(). A kosher rewrite may be to split it into two separate variables, size_t cmdlen = strlen(cmd); ssize_t bytes_read = read(fd, buf, sizeof(buf)-1); > @@ -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; /* lengt of strings */ "length". Again given "size_t strlen(const char *)", this may be sufficiently obvious. > @@ -2001,7 +2004,7 @@ char *mingw_getenv(const char *name) > > int mingw_putenv(const char *namevalue) > { > - int size; > + size_t size; /* lengt of a string */ Ditto. > @@ -3085,7 +3089,8 @@ static void maybe_redirect_std_handles(void) > */ > int wmain(int argc, const wchar_t **wargv) > { > - int i, maxlen, exit_status; > + int i, exit_status; > + size_t maxlen; /*contains length os arguments*/ Missing SP around the words. Again, given "size_t wcslen(const wchar_t *)", it may be obvious to readers. > diff --git a/compat/vcbuild/include/unistd.h b/compat/vcbuild/include/unistd.h > ... > +#ifdef _WIN64 > +typedef __int64 _ssize_t; > +#else > typedef long _ssize_t; > +#endif // _AMD64 It is a bit unusual that "#ifdef X" is not closed with "#endif /* X */". Some folks write "#endif /* !X */" but what I am wondering about is a mismatch between _WIN64 and _AMD64.
Hi Sören On 09/10/2024 18:13, Sören Krecker wrote: > Remove some compiler warnings from msvc in compat/mingw.c for value truncation from 64 bit to 32 bit intigers. Thanks for re-rolling, I think "Fix some compiler warnings" would be clearer than "Remove", also "integers" is misspelt. As Junio said we fold our commit messages at 72 characters. When I said it would be helpful to explain the choice of signed/unsigned I meant an explanation in the commit message, not code comments. I agree with Junio that the remaining ssize_t should be a size_t so the commit message could say something like Use size_t instead of int as all of the changed variables hold the result of strlen() or wcslen() which cannot be negative. It would also be helpful to explain in the commit message the changes to _ssize_t > +#ifdef _WIN64 > +typedef __int64 _ssize_t; > +#else > typedef long _ssize_t; > +#endif // _AMD64 Please note that we do not use "//" comments so this should be "/* _WIN64 */" so that the comment matches the opening #ifdef Thanks for working on this Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks for re-rolling, I think "Fix some compiler warnings" would be > clearer than "Remove", also "integers" is misspelt. Though "Fix" is a word with less information than other words we could use. The changes in the patch are primarily about mismatched type, so perhaps mingw: use size_t insead of int for lengths would make a better commit title. I agree with everything you said including this part: > It would also be helpful to explain in the commit message the changes > to _ssize_t > >> +#ifdef _WIN64 >> +typedef __int64 _ssize_t; >> +#else >> typedef long _ssize_t; >> +#endif // _AMD64 > > Please note that we do not use "//" comments so this should be "/* > _WIN64 */" so that the comment matches the opening #ifdef > > Thanks for working on this Thanks.
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..5293f4cdae 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); /*can become negative*/ 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; + size_t namelen; /* contains length of a string*/ 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; /* read() can return negativ values */ + 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; /* lengt of strings */ wchar_t *w_key; char *value; wchar_t w_value[32768]; @@ -1968,7 +1969,8 @@ 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 +1985,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 +2004,7 @@ char *mingw_getenv(const char *name) int mingw_putenv(const char *namevalue) { - int size; + size_t size; /* lengt of a string */ wchar_t *wide, *equal; BOOL result; @@ -2011,7 +2014,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 +3089,8 @@ static void maybe_redirect_std_handles(void) */ int wmain(int argc, const wchar_t **wargv) { - int i, maxlen, exit_status; + int i, exit_status; + size_t maxlen; /*contains length os arguments*/ char *buffer, **save; const char **argv; diff --git a/compat/vcbuild/include/unistd.h b/compat/vcbuild/include/unistd.h index 3a959d124c..1c0096ab21 100644 --- a/compat/vcbuild/include/unistd.h +++ b/compat/vcbuild/include/unistd.h @@ -14,7 +14,11 @@ typedef _mode_t mode_t; #ifndef _SSIZE_T_ #define _SSIZE_T_ +#ifdef _WIN64 +typedef __int64 _ssize_t; +#else typedef long _ssize_t; +#endif // _AMD64 #ifndef _OFF_T_ #define _OFF_T_
Remove some compiler 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 | 25 +++++++++++++++---------- compat/vcbuild/include/unistd.h | 4 ++++ 3 files changed, 21 insertions(+), 12 deletions(-)