[PATCH 1/3 v3] Cygwin: tzcode resync: basics

[Corinna Vinschen]

Hi Mark,

On May 26 00:09, Mark Geisert wrote:
> Hi Corinna,
> 
> Corinna Vinschen wrote:
> > Hi Mark,
> > 
> > > On May 22 02:32, Mark Geisert wrote:
> > On May 25 14:06, Corinna Vinschen wrote:
> > > > Modifies winsup/cygwin/Makefile.in to build localtime.o from items in
> > > > new winsup/cygwin/tzcode subdirectory.  Compiler option "-fpermissive"
> > > > is used to accept warnings about missing casts on the return values of
> > > > malloc() calls.  This patch also removes existing localtime.cc and
> > > > tz_posixrules.h from winsup/cygwin as they are superseded by the
> > > > subsequent patches in this set.
> > > > [...]
> > > > @@ -246,6 +246,15 @@ MATH_OFILES:= \
> > > >   	tgammal.o \
> > > >   	truncl.o
> > > > +TZCODE_OFILES:=localtime.o
> > > > +
> > > > +localtime.o: $(srcdir)/tzcode/localtime.cc $(srcdir)/tzcode/localtime.c.patch
> > > > +	(cd $(srcdir)/tzcode && \
> > > > +		patch -u -o localtime.c.patched localtime.c localtime.c.patch)
> > > > +	$(CXX) ${CXXFLAGS} ${localtime_CFLAGS} \
> > > > +		-I$(target_builddir)/winsup/cygwin \
> > > > +		-I$(srcdir) -I$(srcdir)/tzcode -c -o $@ $<
> > > > +
> > > 
> > > This doesn't work well for me.  That rule is the top rule in Makefile.in
> > > now, so just calling `make' doesn't build the DLL anymore, only
> > > localtime.o.  The rule should get moved way down Makefile.in.
> 
> Oops.  My workflow didn't make this apparent to me.  Thanks for the fix.
> 
> > > What still bugs me [...etc...]
> > > I attached the followup patches to this mail.  Please scrutinize it and
> > > don't hesitate to discuss the changes.  For a start:
> > > 
> > > - I do not exactly like the name "localtime_wrapper.c" but I don't
> > >    have a better idea.
> 
> localtime_cygwin.c?  cyglocaltime.c?  Not much nicer IMO.
> 
> > > - muto's are C++-only, so I changed rwlock_wrlock/rwlock_unlock to use
> > >    Windows SRWLocks.  I think this is a good thing and I'm inclined
> > >    to drop the muto datatype entirely in favor of using SRWLocks since
> > >    they are cleaner and langauge-agnostic.
> > 
> > Two changes in my patchset:
> > 
> > - I didn't initialize the SRWLOCK following the books.  Fixed that.
> > 
> > - Rather than creating the patched file in the source dir, I changed
> >    the Makefile.in rule so that the patched file is created in the build
> >    dir.  This drops the requirement to tweak .gitignore.  It's also
> >    cleaner.
> > 
> > - Splitting the build rule for localtime.c.patched from the build rule
> >    for localtime.o makes sure that the patched file is not regenerated
> >    every time we build localtime.o.
> > 
> > I attached my patchset again, but only patch 3 and 4 actually changed.
> 
> All the above are great improvements.  But I would now remove the "// Get
> ready to wrap NetBSD's localtime.c" line and blank line following it.

(Belatedly) done.

> Good to go!

Great!  I added two more tweaks:

- I renamed the generated file from localtime.c.patched to
  localtime.patched.c so the .c suffix remains intact.  Seems
  a bit cleaner to me.  I also added it to the 'clean' rule,
  so that it gets removed at `make clean' time.

- I simplified the #includes in the wrapper file.  The paths to these
  headers are searched anyway, so they don't have to be written out
  explicitely.

> Thank you,

Good job, thank you!


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer