This is the mail archive of the
cygwin-apps
mailing list for the Cygwin project.
Re: setup
- From: Achim Gratz <Stromeko at nexgo dot de>
- To: cygwin-apps at cygwin dot com
- Date: Tue, 30 Jun 2015 19:14:29 +0200
- Subject: Re: setup
- Authentication-results: sourceware.org; auth=none
- References: <20150610080526 dot GC31537 at calimero dot vinschen dot de> <871thjtq0m dot fsf at Rainer dot invalid> <20150610185417 dot GL31537 at calimero dot vinschen dot de> <87wpzbs2yj dot fsf at Rainer dot invalid> <87mw060xg3 dot fsf at Rainer dot invalid> <20150612102945 dot GS31537 at calimero dot vinschen dot de> <87bngkzwki dot fsf at Rainer dot invalid> <87ioa8t0ha dot fsf at Rainer dot invalid> <20150629134154 dot GA2918 at calimero dot vinschen dot de> <873819okox dot fsf at Rainer dot invalid> <20150630164052 dot GE2918 at calimero dot vinschen dot de>
Corinna Vinschen writes:
> Ok, for once. But please make sure that you split the commit into
> functional chunks next time it's so large.
Well, the original patch was a lot smaller and I didn't really expect
that I'd have to replace such a large chunk of the old code to make
things work correctly. I've thought about it some more and I guess I
can split off the search implementation in fromcwd. The meat of the
patch would still be quite large, though.
> And send it to this list, so
> code snippets can be referenced in the review.
Sure.
> A few more nits, mostly on style:
>
> - What I'm missing are code comments in do_local_ini and do_remote_ini
> describing what the new methods do. Yes, the old code didn't have a
> lot of comments either, but it would be nice to have this better
> explained for future reference.
I'll add some.
> - Do you plan to keep the DEBUG_FROMCWD bracketed code in there? If so,
> it should probably just use `#if DEBUG' as elsewhere.
I was using similar code from crypto.cc as a guide. I can remove that
code as it's debugged now. In the long run it'd be better to have
Log(DEBUG_...) calls that get optimized out when doing the final build
indtead of conditional compilation.
> - The call to yydebug=1 is almost invisible now. Please revert that
> to the old
>
> [empty line]
> /*yydebug = 1; */
> [empty line]
OK.
> - ini_decompress as a function name may be a bit misleading. What
> about decompress_ini_file instead?
OK.
> Otherwise it looks ok, especially splitting the code into the new
> functions ini_decompress/decompress_ini_file and check_ini_sig and
> removing IniParseFindVisitor is a blessing.
I'd have really liked to refactor local and remote as well as they are
almost exact copies of each other. I didn't try if file:// URLs would
have worked, maybe they do. If so, the search part for the remote init
would need to be lifted so that both code paths would just process a
list of URLs.
Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+
Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves