threads / patch / 20639

patch, 11 partsRemove va_copy at MSVC because there are va_copy.

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

## tl;dr

10 messages between Aug 17, 2009 and Aug 18, 2009. Diffs are folded; open one to read it.

replies: 9people: 6as markdown or json

Frank Li· Aug 17, 2009, 16:04 UTC · lore

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(-)
Show changes to compat/winansi.c +8 −0
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
Johannes Schindelin· Aug 17, 2009, 16:49 UTC · re: Frank Li · lore

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

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:
Show 15 quoted lines
> 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

Joshua Jensen· Aug 17, 2009, 18:22 UTC · re: Johannes Schindelin · lore

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

----- Original Message -----
From: Johannes Schindelin
Date: 8/17/2009 10:49 AM
Show 7 quoted lines
> 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
Paolo Bonzini· Aug 17, 2009, 16:53 UTC · re: Frank Li · lore

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

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
Reece Dunn· Aug 17, 2009, 16:56 UTC · re: Paolo Bonzini · lore

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

2009/8/17 Paolo Bonzini <bonzini@gnu.org>:
Show 15 quoted lines
> 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
Erik Faye-Lund· Aug 17, 2009, 17:02 UTC · re: Paolo Bonzini · lore

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

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
Erik Faye-Lund· Aug 17, 2009, 17:26 UTC · re: Erik Faye-Lund · lore

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

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
Johannes Schindelin· Aug 17, 2009, 19:46 UTC · re: Erik Faye-Lund · lore

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

Hi,
On Mon, 17 Aug 2009, Erik Faye-Lund wrote:
Show 10 quoted lines
> 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

Frank Li· Aug 18, 2009, 05:06 UTC · re: Paolo Bonzini · lore

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

Show 9 quoted lines
>
> #ifndef va_copy
> #define va_copy(dst, src)	((dst) = (src))
> #endif
>
> if it works on MSVC?
>
> Paolo
>
I test it, it works.
Johannes Schindelin· Aug 18, 2009, 09:54 UTC · re: Frank Li · lore

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

Hi,
On Tue, 18 Aug 2009, Frank Li wrote:
Show 7 quoted lines
> > #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

← back to recent threads