Re: [msysGit] [PATCH] mingw: redefine the wrapper macro after the corresponding function
Hello, On Fri, Jun 06, 2014 at 12:00:51AM +0200, Karsten Blees wrote: Am 05.06.2014 18:56, schrieb Johannes Sixt: Within mingw.c, if some other function inside mingw.c wants to use mingw_unlink, then it should be written as 'mingw_unlink(foo)', not 'unlink(foo)'. I very much like this approach. In fact, we already do this for e.g. mingw_raise. Hannes, this is consistent with your commit 06bc4b7. Settled. Other callers would typically want the wrapped version (i.e. mingw_*). If this assumption were true, then we have to keep the wrapper macros defined, both above and below the wrapper function definition. You are in fact advocating my patch. Updated version follows. Stepan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] mingw: redefine the wrapper macro after the corresponding function
Am 06.06.2014 10:32, schrieb Stepan Kasal:
Hello,
On Fri, Jun 06, 2014 at 12:00:51AM +0200, Karsten Blees wrote:
Am 05.06.2014 18:56, schrieb Johannes Sixt:
Within mingw.c, if some other function inside mingw.c wants to use
mingw_unlink, then it should be written as 'mingw_unlink(foo)', not
'unlink(foo)'.
I very much like this approach. In fact, we already do this for e.g.
mingw_raise.
Hannes, this is consistent with your commit 06bc4b7. Settled.
Other callers would typically want the wrapped version (i.e.
mingw_*).
If this assumption were true, then we have to keep the wrapper macros
defined, both above and below the wrapper function definition.
That's not what I meant. Assume all other callers are written 'mingw_foo', as
suggested by Hannes, and no one except 'mingw_foo' has the need to call
MSVCRT's 'foo' directly. Then its irrelevant whether the #undef is at the top
or immediately before 'mingw_foo'. Having the #undef in close vicinity of the
function definition helps removing it when its no longer needed.
Thinking about this some more, the best solution is probably to eliminate the
problem altogether by adding inline-wrappers for required CRT-functions, e.g.:
mingw.h:
static inline int crt_gethostname(char *host, int namelen)
{
return gethostname(host, namelen);
}
int mingw_gethostname(char *host, int namelen);
#define gethostname mingw_gethostname
mingw.c:
int mingw_gethostname(char *name, int namelen)
{
ensure_socket_initialization();
return crt_gethostname(name, namelen);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] mingw: redefine the wrapper macro after the corresponding function
Hi Karsten,
On Fri, Jun 06, 2014 at 11:43:03AM +0200, Karsten Blees wrote:
[...] Assume all other callers are written
'mingw_foo', as suggested by Hannes, and no one except 'mingw_foo'
has the need to call MSVCRT's 'foo' directly. Then its irrelevant
whether the #undef is at the top or immediately before 'mingw_foo'.
Yet there is still danger that someone calls foo() by mistake.
It is still best to have a protection:
#define foo choke_here_do_not_use_this
Thinking about this some more, the best solution is probably to
eliminate the problem altogether by adding inline-wrappers for
required CRT-functions, e.g.:
Yes, this is acceptable. But I wouldn't pollute mingw.h. You can do
it on top of mingw.c like this:
#undef gethostname
static inline int crt_gethostname(char *host, int namelen)
{
return gethostname(host, namelen);
}
#define gethostname please_call_the_mingw_or_crt_version
This would also be an acceptable solution, though I still prefer my
solution, because, as you put it:
Having the #undef in close vicinity of the function definition
helps removing it when it's no longer needed.
Stepan
PS: Anyway, this is another patch which I can mark as too much
discussion, try later. Then I can proceed and submit your unicode
branch.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] mingw: redefine the wrapper macro after the corresponding function
Am 06.06.2014 13:10, schrieb Stepan Kasal:
Hi Karsten,
On Fri, Jun 06, 2014 at 11:43:03AM +0200, Karsten Blees wrote:
Thinking about this some more, the best solution is probably to
eliminate the problem altogether by adding inline-wrappers for
required CRT-functions, e.g.:
Yes, this is acceptable. But I wouldn't pollute mingw.h. You can do
it on top of mingw.c like this:
But having it in the .h file may come in handy if we want to split the overlong
mingw.c into several compilation units...
#undef gethostname
static inline int crt_gethostname(char *host, int namelen)
{
return gethostname(host, namelen);
}
#define gethostname please_call_the_mingw_or_crt_version
Now you're mixing all three variants...note that with my suggestion to #define
crt_foo in mingw.h, you don't need '#undef foo', nor redefine foo (your
variant), nor rename other callers in mingw.c to 'mingw_foo' (Hannes' variant).
Callers of foo() would simply write foo(), no matter whether in mingw.c or
anywhere else. In the special case that you really want the CRT version, you'd
write crt_foo(). This works everywhere, even in core-git code wrapped in #ifdef
GIT_WINDOWS_NATIVE.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] mingw: redefine the wrapper macro after the corresponding function
Am 05.06.2014 10:05, schrieb Stepan Kasal: mingw.c defines several wrapper functionsi, like mingw_unlink(). These wrappers are deployed by macros like this: #define unlink mingw_unlink The function itself is preceded by #undef, leaving the wrapper out of the game for the rest of mingw.c. This was not probably intentional; for example, there are three calls to open() below the definition mingw_open() that probably have no reason to circumvent the wrapper. OTOH, there is one call to gethostbyname() before it was undefined; probably happy that it actually calls mingw_gethostbyname(). This patch adds back the #define after each wrapper definition. Signed-off-by: Stepan Kasal ka...@ucw.cz --- compat/mingw.c | 20 1 file changed, 20 insertions(+) diff --git a/compat/mingw.c b/compat/mingw.c index a0e13bc..e7193c0 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -224,6 +224,7 @@ int mingw_unlink(const char *pathname) ret = unlink(pathname); return ret; } +#define unlink mingw_unlink (etc...) I don't particularly like this approach: It robs the precise control of which function we can invoke from other places in mingw.c. Within mingw.c, if some other function inside mingw.c wants to use mingw_unlink, then it should be written as 'mingw_unlink(foo)', not 'unlink(foo)'. So, IMO the macros should be #undef'ed at the top of the file, and all users (like the open() and gethostbyname() invocations that you identified) should be audited and changed to call the function they actually need (i.e., the system open vs. mingw_open). -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] mingw: redefine the wrapper macro after the corresponding function
Am 05.06.2014 18:56, schrieb Johannes Sixt: Am 05.06.2014 10:05, schrieb Stepan Kasal: mingw.c defines several wrapper functionsi, like mingw_unlink(). These wrappers are deployed by macros like this: #define unlink mingw_unlink The function itself is preceded by #undef, leaving the wrapper out of the game for the rest of mingw.c. This was not probably intentional; for example, there are three calls to open() below the definition mingw_open() that probably have no reason to circumvent the wrapper. OTOH, there is one call to gethostbyname() before it was undefined; probably happy that it actually calls mingw_gethostbyname(). This patch adds back the #define after each wrapper definition. Signed-off-by: Stepan Kasal ka...@ucw.cz --- compat/mingw.c | 20 1 file changed, 20 insertions(+) diff --git a/compat/mingw.c b/compat/mingw.c index a0e13bc..e7193c0 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -224,6 +224,7 @@ int mingw_unlink(const char *pathname) ret = unlink(pathname); return ret; } +#define unlink mingw_unlink (etc...) I don't particularly like this approach: It robs the precise control of which function we can invoke from other places in mingw.c. Within mingw.c, if some other function inside mingw.c wants to use mingw_unlink, then it should be written as 'mingw_unlink(foo)', not 'unlink(foo)'. I very much like this approach. In fact, we already do this for e.g. mingw_raise. So, IMO the macros should be #undef'ed at the top of the file, and all users (like the open() and gethostbyname() invocations that you identified) should be audited and changed to call the function they actually need (i.e., the system open vs. mingw_open). I'm sceptical of moving all #undef's to the top. Other callers would typically want the wrapped version (i.e. mingw_*). At least I can't think of a scenario in which a higher level function would want to bypass the wrapper... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
