definition for _attribute() in remote.c

Hi,

I'm looking at getting Git for Windows to compile via Visual Studio 
(https://github.com/git-for-windows/git/pull/256).

However the use of __attribute() in remote.c at L1662 
(https://github.com/git-for-windows/git/blob/master/remote.c#L1662) has got 
me confused in that I can't see how the regular definition of __attribute() 
is #included in this case. A definition is given in 
git\compat\regex\regex_internal.h but doesn't appear to be on remote.c's 
include path.

The line was introduced by 3a429d0 (remote.c: report specific errors from 
branch_get_upstream, 2015-05-21) which appears to be later than the previous 
MSVC testers had looked at.

Any guidance on what is happening in this case would be welcomed as this is 
the last compile error I'm seeing.

--

Philip

On Mon, Apr 25, 2016 at 10:02:38PM +0100, Philip Oakley wrote:

I'm looking at getting Git for Windows to compile via Visual Studio
(https://github.com/git-for-windows/git/pull/256).

However the use of __attribute() in remote.c at L1662
(https://github.com/git-for-windows/git/blob/master/remote.c#L1662) has got
me confused in that I can't see how the regular definition of __attribute()
is #included in this case. A definition is given in
git\compat\regex\regex_internal.h but doesn't appear to be on remote.c's
include path.

The line was introduced by 3a429d0 (remote.c: report specific errors from
branch_get_upstream, 2015-05-21) which appears to be later than the previous
MSVC testers had looked at.

It should be handled in git-compat-util.h, which is included by cache.h,
which is included by remote.c.

There we have:

  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif

which should make it a noop on compilers which don't know about it. Is
VS (or another file) setting __GNUC__?

-Peff

On Mon, Apr 25, 2016 at 05:10:30PM -0400, Jeff King wrote:

It should be handled in git-compat-util.h, which is included by cache.h,
which is included by remote.c.

There we have:

  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif

which should make it a noop on compilers which don't know about it. Is
VS (or another file) setting __GNUC__?

Of course it helps if we spell the name right...

-- >8 --
Subject: remote.c: spell __attribute__ correctly

We want to tell the compiler that error_buf() uses
printf()-style arguments via the __attribute__ mechanism,
but the original commit (3a429d0), forgot the trailing "__".
This happens to work with real GNUC-compatible compilers
like gcc and clang, but confuses our fallback macro in
git-compat-util.h, which only matches the official name (and
thus the build fails on compilers like Visual Studio).

Reported-by: Philip Oakley <redacted>
Signed-off-by: Jeff King <redacted>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 28fd676..ddc4f8f 100644
--- a/remote.c
+++ b/remote.c

@@ -1660,7 +1660,7 @@ int branch_merge_matches(struct branch *branch,
 	return refname_match(branch->merge[i]->src, refname);
 }
 
-__attribute((format (printf,2,3)))
+__attribute__((format (printf,2,3)))
 static const char *error_buf(struct strbuf *err, const char *fmt, ...)
 {
 	if (err) {

-- 
2.8.1.562.gc7e1b3c

From: "Jeff King" <redacted>

On Mon, Apr 25, 2016 at 10:02:38PM +0100, Philip Oakley wrote:

quoted

I'm looking at getting Git for Windows to compile via Visual Studio
(https://github.com/git-for-windows/git/pull/256).

However the use of __attribute() in remote.c at L1662
(https://github.com/git-for-windows/git/blob/master/remote.c#L1662) has 
got
me confused in that I can't see how the regular definition of 
__attribute()
is #included in this case. A definition is given in
git\compat\regex\regex_internal.h but doesn't appear to be on remote.c's
include path.

The line was introduced by 3a429d0 (remote.c: report specific errors from
branch_get_upstream, 2015-05-21) which appears to be later than the 
previous
MSVC testers had looked at.

It should be handled in git-compat-util.h, which is included by cache.h,
which is included by remote.c.

There we have:

 #ifndef __GNUC__
 #ifndef __attribute__
 #define __attribute__(x)
 #endif
 #endif

which should make it a noop on compilers which don't know about it. Is
VS (or another file) setting __GNUC__?

It's not the __attribute__ definition (a Gnu C ism), rather its the 
__attribute variant, which has a definition in regex_internal.h, and is used 
in the regex code. It's that one that's used in remote.c that I can't fathom 
(i.e. how it worked in normally)

regex_internal.h#L160-164
#ifdef __GNUC__
# define __attribute(arg) __attribute__ (arg)
#else
# define __attribute(arg)
#endif

thus when the compilation get to remote.c#L1662 it fails to find that 
definition.

Should that line use the gnu extension name?

--
Philip 

On Mon, Apr 25, 2016 at 10:34:27PM +0100, Philip Oakley wrote:

It's not the __attribute__ definition (a Gnu C ism), rather its the
__attribute variant, which has a definition in regex_internal.h, and is used
in the regex code. It's that one that's used in remote.c that I can't fathom
(i.e. how it worked in normally)

regex_internal.h#L160-164
#ifdef __GNUC__
# define __attribute(arg) __attribute__ (arg)
#else
# define __attribute(arg)
#endif

thus when the compilation get to remote.c#L1662 it fails to find that
definition.

Should that line use the gnu extension name?

Yeah, sorry, see my followup response. We don't use "__attribute" at
all ourselves. The reference you found is in compat code that we pulled
in (so it does its own thing entirely), and the one in remote.c is just
a typo/think-o that happened to work on gcc, et al.

-Peff

From: "Jeff King" <redacted>

Thanks for clarifying that (sorry about the crossed emails). The compile is 
now looking good.
I'm just left with some unresolved external symbol link errors now.

The same naming issue in compat/regex/regcomp.c, compat/regex/regexec.c, 
compat/regex/regex_internal.c and compat/regex/regex_internal.h  was 
probably what lead me astray...

Philip

On 25/04/16 22:50, Philip Oakley wrote:

From: "Jeff King" <redacted>

quoted

On Mon, Apr 25, 2016 at 05:10:30PM -0400, Jeff King wrote:

quoted

It should be handled in git-compat-util.h, which is included by cache.h,
which is included by remote.c.

There we have:

  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif

which should make it a noop on compilers which don't know about it. Is
VS (or another file) setting __GNUC__?

Of course it helps if we spell the name right...

Indeed! ;-)

Not that it matters, but the above #define in git-compat-util.h is not
the relevant definition - msvc will not see it. However, it does see
the #define on line 12 of compat/msvc.h. :-D

ATB,
Ramsay Jones

Thnx,
From: "Ramsay Jones" <redacted>

On 25/04/16 22:50, Philip Oakley wrote:

quoted

From: "Jeff King" <redacted>

quoted

On Mon, Apr 25, 2016 at 05:10:30PM -0400, Jeff King wrote:

quoted

It should be handled in git-compat-util.h, which is included by 
cache.h,
which is included by remote.c.

There we have:

  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif

which should make it a noop on compilers which don't know about it. Is
VS (or another file) setting __GNUC__?

Of course it helps if we spell the name right...

Indeed! ;-)

Not that it matters, but the above #define in git-compat-util.h is not
the relevant definition - msvc will not see it.

Ah, I see that that block is further guarded with other if/elif/else clauses 
so that it's not seen if _MSC_VER is defined.

git-compat-util.h#L400-411

However, it does see
the #define on line 12 of compat/msvc.h. :-D

ATB,
Ramsay Jones