This is the mail archive of the cygwin-apps@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Postinstall script ordering in setup


Rob,

Thanks for the review.  Comments below...

On 4 Mar 2003, Robert Collins wrote:

> On Tue, 2003-03-04 at 15:36, Igor Pechtchanski wrote:
> > Hi,
> >
> > This patch adds a dependence tracking mechanism to postinstall scripts.
> > The idea is essentially the one described in
> > <http://cygwin.com/ml/cygwin-apps/2003-03/msg00022.html>.  This patch is
> > very preliminary, and not tested much beyond its ability to sort scripts
> > correctly (compiles, though).  This should provide a foundation for
> > further refinement, however.  Please comment.
>
> Commenting from the changelog for now...
>
> I'll do a more indepth review when the obvious issues below are sorted
> out. BTW: I agree with this concept as a short term solution.
>
> >       Igor
> > ==============================================================================
> > ChangeLog:
> > 2003-03-03  Igor Pechtchanski <pechtcha at cs dot nyu dot edu>
> >
> >       * postinstall.cc (RunFindVisitor::executeAll): New
> >       member function that propagates script dependences,
> >       topologically sorts the script list, and then executes
> >       the scripts.
>
> This does too much. When a method does more than one thing... split it
> up.

This does it by calling 3 functions.  It's a 7-line function. :-D
I guess my eagerness to make the changelog detailed enough got the best of
me.

> >       (RunFindVisitor::visitFile): Add file to list instead of
> >       running it.
> >       (RunFindVisitor::files): New member variable.
> >       (processFile): New static helper function that extracts
> >       dependences from each script file.
>
> I suspect at this point we may want a class to handle this - to extract,
> remember and sort dependencies.

There is a class that contains the dependences (FileDesc).  I could put
this function in that class (as static, though, so doesn't change much).

> >       (do_postinstall): Add executeAll call.
> >       (FileDesc): New helper class.
> >       (sharpbang): New static helper function.
>
> This should be in a class, IMO.
> Rob

I've forgotten a lot of C++ in the recent years.  In Java, I would have
made this a private method.  How expensive are non-virtual private
functions in C++?  If same as regular functions, I'll put it in the class
(along with processFile).
	Igor
-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_		pechtcha at cs dot nyu dot edu
ZZZzz /,`.-'`'    -.  ;-;;,_		igor at watson dot ibm dot com
     |,4-  ) )-,_. ,\ (  `'-'		Igor Pechtchanski
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

Oh, boy, virtual memory! Now I'm gonna make myself a really *big* RAMdisk!
  -- /usr/games/fortune


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]