Re: [PATCH v2 1/1] [PATCH] mimgw: remove Compiler Warnings
From: Junio C Hamano <hidden>
Date: 2024-10-09 18:20:49
Sören Krecker [off-list ref] 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?
quoted hunk
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.
quoted hunk
@@ -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.
quoted hunk
@@ -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);
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
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.