setup
Corinna Vinschen
corinna-cygwin@cygwin.com
Tue Jun 30 18:42:00 GMT 2015
On Jun 30 19:14, Achim Gratz wrote:
> 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.
Thanks.
> > - 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.
If you're *really* sure we don't need the debug statements anymore,
then, yes, remove them.
> 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.
ACK
> > - 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.
Sounds good to me for some later patch.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-apps/attachments/20150630/65c27f48/attachment.sig>
More information about the Cygwin-apps
mailing list