OK.. so how's this:
For close I'll just edit in your sys_close hack :)
For sys_open, let's just unconditionally say:
int imode = va_arg (ap, int); mode=(mode_t)imode;
without the surrounding if(sizeof(mode_t) < sizeof(int)).
I'll hack that in by hand and push to see how it floats :0
M
On Tue, Dec 18, 2012 at 12:35:07PM -0500, Hans-Christoph Steiner wrote:
Windows is most definitely not POSIX compliant. If it was, we wouldn't be having this discussion since the Win32 open() would just work. It has lots POSIX compliant things, but is also missing many key ones. For example, WIN32 does not define any of the POSIX open() flags (O_CREAT, O_TRUNC, etc.) but instead uses _O_CREAT, _O_TRUNC, etc. The WIN32 open() permissions flags its uses work differently than POSIX.
Microsoft marked close() as deprecated in 2005. It seems worthwhile to heed that.
The other issue with this patch is the giant warning it gives on Mac OS X: s_path.c: In function ‘sys_open’: s_path.c:456: warning: ‘mode_t’ is promoted to ‘int’ when passed through ‘...’ s_path.c:456: warning: (so you should pass ‘int’ not ‘mode_t’ to ‘va_arg’) s_path.c:456: note: if this code is reached, the program will abort
.hc
On Dec 18, 2012, at 12:28 PM, Miller Puckette wrote:
... but if POSIX has a close() I think there's no issue here - MSW is POSIX compliant, they say, and hence they're committeed to maintaining close(). So I think it's fine just to use close() and not have a sys_close() at all (or if someone is actually using sys_close() we choud just:
int sys_close(int fd) { return close(fd); }
:) M
On Tue, Dec 18, 2012 at 12:22:29PM -0500, Hans-Christoph Steiner wrote:
On Dec 18, 2012, at 4:56 AM, IOhannes m zmölnig wrote:
On 12/18/2012 04:40, Hans-Christoph Steiner wrote:
I think this approach works.
thanks
The patch you provided seems totally untested, as in not even compiled on GNU/Linux or Mac OS X. It includes the _close() function in sys_close() which only works on Win32 and it gives this warning when building on Mac OS X:
thanks for the compliments :-)
i can assure you that the code is tested as in "compiled without warning on gcc-4.7.2 on a debian/gnu linux (wheezy/sid) system" and has been field-tested and shown to be able to load externals that the old code has not been able to load.
however, you are of course right that the use of "_close()" is indeed an oversight, which only did not trigger a problem so far, as sys_close() is nowhere used yet.
s_path.c: In function ‘sys_open’: s_path.c:450: warning: ‘mode_t’ is promoted to ‘int’ when passed through ‘...’ s_path.c:450: warning: (so you should pass ‘int’ not ‘mode_t’ to ‘va_arg’) s_path.c:450: note: if this code is reached, the program will abort
the patch includes some comments pointing to an online discussion of the problem. to summarize: using mode_t in va_list will always trigger some problems. either we accept the warning (the code will never be reached, since a runtime-test will use a va_arg(..., int) instead) or we move the test to configure (autoconf).
since i am the only one who seems to like autoconf, i decided for the less invasive solution.
I think it makes sense to restore sys_close() for backwards ABI compatibility. Microsoft says that the POSIX close() was deprecated in 2005, and to use their ISO C++ _close() instead [1][2], so the new sys_close() should look like this:
/* close a previously opened file this is needed on platforms where you cannot open/close resources across dll-boundaries */ int sys_close(int fd) { #ifdef _WIN32 return _close(fd); #else return close(fd); #endif }
And leave sys_open, sys_fopen, and sys_fclose as macros in the header. This implementation of sys_open and warning are much more complicated. And tho normally I think its good to avoid #ifdefs in headers, in this case I think it actually communicates why we have these sys_open/sys_close sys_fopen/sys_fclose in the first place: "Win32 lacks good POSIX API support, everything else works as is".
Attached is my patch that I think should replace 2b8a4c13904f6b8bef3a8ae52b5258a131eb6a19 "provide sys_close(),... on all platforms" .hc
[1] http://msdn.microsoft.com/en-US/library/ms235443(v=vs.80).aspx [2] http://msdn.microsoft.com/en-US/library/5fzwd5ss(v=vs.80).aspx
(I attached the patch for reference, it doesn't seem to be up on the patch tracker any more.)
it seems that the patch has moved to the "bug" tracker. i moved it back to "patches" [1].
fgmasdr IOhannes
[1] https://sourceforge.net/tracker/?func=detail&aid=3596865&group_id=55...
Pd-dev mailing list Pd-dev@iem.at http://lists.puredata.info/listinfo/pd-dev
Pd-dev mailing list Pd-dev@iem.at http://lists.puredata.info/listinfo/pd-dev