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