Re: [PATCH] system-type cygwin with window-system w32

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sun, 17 Jul 2011 23:29:19 -0700
> From: Daniel Colascione <dan.colascione@gmail.com>
> CC: emacs-devel@gnu.org
> 
> On 7/17/11 11:13 PM, Eli Zaretskii wrote:
> > Why did you need to change filemode.c?  Does it have anything to do
> > with Cygwin on w32?
> 
> S_ISCTG and such aren't being defined under Cygwin, causing compilation
> errors.  There's probably a better way to deal with the underlying problem.

Yes, the files in lib/sys_stat.in.h is supposed to do that already.
I'm curious why it didn't work for you.

> > What is this (and related) stuff about?  Why do you need to use HTML
> > wrt the clipboard?
> 
> Windows uses HTML as a data interchange format --- supporting it as a
> clipboard format allows formatting to be preserved in pastes into other
> programs.  This code could easily be structured as a separate package,
> however, and I'll end up doing that.

Yes, let's have this as a separate changeset.

> > I don't like the idea of moving this to w32fns.c, because it doesn't
> > belong there.  Can you come up with an alternative idea?
> 
> The fundamental problem is that we now have two Windows platforms:
> WINDOWSNT and (CYGWIN && HAVE_NTGUI).  The common code has to live
> somewhere; with my patch, we only build w32.o in the NTEMACS case
> because w32.c contains mostly compatibility wrappers; the
> non-compatibility portions I moved to w32fns.c, which we compile in both
> cases.  Cygwin-NT-specific code goes in the new file cygw32.c.
> 
> Another option would be to further refactor the Win32 code into distinct
> and explicit WINDOWSNT and HAVE_NTGUI files and introduce common headers
> for common functionality.  This approach would involve even more code
> movement, however, which is why I initially avoided it.

I'd prefer a separate file common to w2 and Cygwin-on-w32, if that's
needed.  w32fns.c tries to be as similar to xfns.c as possible, so
putting there stuff that's not relevant would be a disadvantage.

> >> +#define t(...)                                          \
> >> +    ({                                                  \
> >> +      fprintf (stderr, "T:%s:%u: ",                     \
> >> +               __FUNCTION__, __LINE__);                 \
> >> +      fprintf (stderr, __VA_ARGS__);                    \
> >> +      fputc ('\n', stderr);                             \
> >> +    })
> >> +
> > 
> > What is this stuff about?
> 
> Debug scaffolding --- in this case, generally useful, I think, at least
> as a replacement for the numerous bespoke tracing macros scattered
> everywhere in the code.

Fine, but (a) please see if there's no macro already available that
can be used instead; and (b) let's have this a separate changeset.

> >> +      /* DebPrint (("w32_msg_pump: %s time:%u\n", */
> >> +      /*            w32_name_of_message (msg.message), msg.time)); */
> >> +      
> > 
> > Can this be removed?  These DebPrint messages are a PITA when
> > debugging, so if it isn't absolutely necessary, let's not add new
> > ones.
> 
> Sure.  I'd actually prefer, though, to leave the existing tracing, but
> move it all to a common macro so that the debug spam is easier to
> disable and enable as needed.

Fine with me.

> > This is based on reviewing only a part of the patch, I will have more
> > later.  The patch is very large and complicated, and the lack of a
> > ChangeLog that describes the changes, particularly those which move
> > code between different files, does not help...
> 
> Of course. It's a work in progress --- a first stab, really.  Once I
> clean up the code a bit, I'll put it into a form that's easier to consume.

My point was that there are several issues here that need to be
discussed before you invest too much energy into them.  So please
consider starting these discussion sooner rather than later, and the
additional information I didn't find in the ChangeLog would be
instrumental at that time.

TIA