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

[Mark Geisert]

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 is that we get these -fpermissive warnings (albeit
>> non-fatal) and the fact that we don't get a dependencies file.  On
>> second thought, there's no good reason to keep localtime.cc a C++ file.
>> Converting this file to a plain C wrapper drops the C++-specific warning
>> and thus allows to revert the localtime.o build rule to use ${COMMON_CFLAGS}.
>>
>> So I took the liberty to tweak your patch a bit.  I created a followup
>> patchset, which I'd like you to take a look at.
>>
>> 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.  Good to go!
Thank you,

..mark