Re: [PATCH v2 1/1] [PATCH] mimgw: remove Compiler Warnings

[Junio C Hamano <gitster@pobox.com>]

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.