Re: [PATCH v2 1/1] [PATCH] mimgw: remove Compiler Warnings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 9, 2024, 18:20 UTC
- Message-ID
- <xmqq4j5lt9xe.fsf@gitster.g>
- In-Reply-To
- <20241009171342.2354-2-soekkle@freenet.de>
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?
Show 10 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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);
Show 6 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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.
Show 7 quoted lines
> @@ -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.
Show 7 quoted lines
> 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.