[PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Frank Li]

MSVs have not implemented va_copy. remove va_copy at MSVC environment.
It will malloc buffer each time.

Signed-off-by: Frank Li <lznuaa@gmail.com>
---
 compat/winansi.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/compat/winansi.c b/compat/winansi.c
index 9217c24..6091138 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -3,7 +3,11 @@
  */
 
 #include <windows.h>
+#ifdef _MSC_VER
+#include <stdio.h>
+#else
 #include "../git-compat-util.h"
+#endif
 
 /*
  Functions to be wrapped:
@@ -310,9 +314,13 @@ static int winansi_vfprintf(FILE *stream, const char *format, va_list list)
 	if (!console)
 		goto abort;
 
+#ifndef _MSC_VER 
 	va_copy(cp, list);
 	len = vsnprintf(small_buf, sizeof(small_buf), format, cp);
 	va_end(cp);
+#else
+	len= sizeof(small_buf) ;
+#endif
 
 	if (len > sizeof(small_buf) - 1) {
 		buf = malloc(len + 1);
-- 
1.6.4.msysgit.0

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Johannes Schindelin]

Hi,

On Tue, 18 Aug 2009, Frank Li wrote:

> MSVs have not implemented va_copy. remove va_copy at MSVC environment.
> It will malloc buffer each time.
> 
> Signed-off-by: Frank Li <lznuaa@gmail.com>

How about this instead?

	Work around Microsoft Visual C++ not having va_copy()

	In winansi.c, Git wants to know the length of the formatted string 
	so it can allocate enough space for it.  But Microsoft Visual C++
	does not have va_copy(), so we have to guess.

The problem is the guessing part:

> diff --git a/compat/winansi.c b/compat/winansi.c
> index 9217c24..6091138 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -310,9 +314,13 @@ static int winansi_vfprintf(FILE *stream, const char *format, va_list list)
>  	if (!console)
>  		goto abort;
>  
> +#ifndef _MSC_VER 
>  	va_copy(cp, list);
>  	len = vsnprintf(small_buf, sizeof(small_buf), format, cp);
>  	va_end(cp);
> +#else
> +	len= sizeof(small_buf) ;
> +#endif

small_buf only is 256 bytes.  How do you want to make sure that the 
subsequent vsnprintf() is not writing outside of the buffer?

Also, you still miss a space between "len" and "=".

Ciao,
Dscho

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Paolo Bonzini]

On 08/17/2009 06:04 PM, Frank Li wrote:
> MSVs have not implemented va_copy. remove va_copy at MSVC environment.
> It will malloc buffer each time.

... but only a 257-byte buffer as dscho pointed out.

In many places that do not have va_copy, a simple assignment works.  And 
va_end is almost always a no-op. So what about

#ifndef va_copy
#define va_copy(dst, src)	((dst) = (src))
#endif

if it works on MSVC?

Paolo

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Reece Dunn]

2009/8/17 Paolo Bonzini <bonzini@gnu.org>:
> On 08/17/2009 06:04 PM, Frank Li wrote:
>>
>> MSVs have not implemented va_copy. remove va_copy at MSVC environment.
>> It will malloc buffer each time.
>
> ... but only a 257-byte buffer as dscho pointed out.
>
> In many places that do not have va_copy, a simple assignment works.  And
> va_end is almost always a no-op. So what about
>
> #ifndef va_copy
> #define va_copy(dst, src)       ((dst) = (src))
> #endif
>
> if it works on MSVC?

According to http://stackoverflow.com/questions/558223/vacopy-porting-to-visual-c
that should work.

- Reece

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Erik Faye-Lund]

On Mon, Aug 17, 2009 at 6:53 PM, Paolo Bonzini<bonzini@gnu.org> wrote:
> #ifndef va_copy
> #define va_copy(dst, src)       ((dst) = (src))
> #endif

Are you sure va_copy is always a preprocessor symbol? How about

#ifdef _MSC_VER
#define va_copy(dst, src)       ((dst) = (src))
#endif

instead? It'd make me sleep slightly better at night, at least ;)

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Erik Faye-Lund]

On Mon, Aug 17, 2009 at 7:02 PM, Erik Faye-Lund<kusmabite@googlemail.com> wrote:
> Are you sure va_copy is always a preprocessor symbol?

According to the following forum-post we are:
http://www.velocityreviews.com/forums/showpost.php?p=1689162&postcount=2

However, I decided to dig a bit further, so I had a look at the public
draft spec at http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf,
section 7.15.1:

"The va_start and va_arg macros described in this subclause shall be implemented
as macros, not functions. It is unspecified whether va_copy and va_end
are macros or
identifiers declared with external linkage."

I don't have access (that I know of) to the finalized spec, but it
looks sketchy to me to depend on va_copy being implemented as a macro
given this wording.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Joshua Jensen]

----- Original Message -----
From: Johannes Schindelin
Date: 8/17/2009 10:49 AM
> How about this instead?
>
> 	Work around Microsoft Visual C++ not having va_copy()
>
> 	In winansi.c, Git wants to know the length of the formatted string 
> 	so it can allocate enough space for it.  But Microsoft Visual C++
> 	does not have va_copy(), so we have to guess
I did not look at the surrounding code, but could Microsoft's C runtime 
extension _vscprintf, which returns the number of characters in the 
formatted string, be of use here?

Josh

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 467 bytes --]

Hi,

On Mon, 17 Aug 2009, Erik Faye-Lund wrote:

> On Mon, Aug 17, 2009 at 6:53 PM, Paolo Bonzini<bonzini@gnu.org> wrote:
> > #ifndef va_copy
> > #define va_copy(dst, src)       ((dst) = (src))
> > #endif
> 
> Are you sure va_copy is always a preprocessor symbol? How about
> 
> #ifdef _MSC_VER
> #define va_copy(dst, src)       ((dst) = (src))
> #endif

Why not #define it in compat/msvc.h?  Or introduce a 
DEFINE_VA_COPY_TRIVIALLY symbol or some such?

Ciao,
Dscho

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Frank Li]

>
> #ifndef va_copy
> #define va_copy(dst, src)	((dst) = (src))
> #endif
>
> if it works on MSVC?
>
> Paolo
>

I test it, it works.

Re: [PATCH 05/11] Remove va_copy at MSVC because there are va_copy.

[Johannes Schindelin]

Hi,

On Tue, 18 Aug 2009, Frank Li wrote:

> > #ifndef va_copy
> > #define va_copy(dst, src)	((dst) = (src))
> > #endif
> >
> > if it works on MSVC?
> 
> I test it, it works.

But please, either put it into compat/msvc.h or make it dependent on some 
#define such as "DEFINE_VA_COPY_TRIVIALLY" so that other platforms who 
might miss va_copy (but can use the trivial definition above) can use it.  
I do not think that va_copy can be defined like this in general.

Ciao,
Dscho