This is the mail archive of the cygwin-apps 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] Incidental setup.exe patches #3: Simplify packagedb task handling


On 14/04/2010 09:21, Corinna Vinschen wrote:

>>   [ ... ]  I couldn't work
>> out what the original problem referred to might have been

> Are you sure?  Please check again.  The original situation which this
> was supposed to fix is this:
> 
> - You have an existing installation in C:\cygwin
> - You want to create a new installation in D:\cygwin-new
> - You choose D:\cygwin-new as root dir in the root dir dialog
> - The package list was nevertheless based on the c:\cygwin directory
>   installation, because the package_db stuff was already initialized
>   with the Cygwin root dir set to C:\cygwin

  Thanks, that's just what I was looking for.

> Can you make sure that this doesn't happen after your change?

  Argh, zOMG.  See, I did this:

> $ grep -w task *.[ch]*
> archive.cc: * One such task is identifying archives
> compress.cc: * One such task is identifying compresss
> package_db.cc:  packagedb::task =
> package_db.h:  static PackageDBActions task;
> package_meta.cc:      return db.task == PackageDB_Install ? "Reinstall" : "Retri
> eve";
> root.cc:  db.task = chosen_db_task;
> threebar.cc:  switch (task)
> threebar.cc:  Window::PostMessage (task);
> threebar.h:  int task;
> threebar.h:    task = t;

.... and reasoned that there really is only one place the behaviour of the
application can possibly depend on the value set in task.  Then I looked at this:

> -  /* Deferred initialization of packagedb *after* the root dir has been
> -     chosen. */
> -  packagedb db;
> -  db.task = chosen_db_task;

and thought "Oh, and we don't need to actually construct a packagedb and throw
it away again just to access a static member".

  *Sigh*, I should have read what the constructor does.  Wow, what a
misbegotten application of OOP that is.  A pseudo-singleton that stores all
the data in class static members the first time you construct one and
retrieves it for any subsequently constructed ones.  That's probably the first
non-idempotent default constructor I've ever seen.

  So:

1.  There's no problem setting the class-static task variable early: it's just
constructing the first packagedb that mustn't be done until after the root is set.
1a. So we can still get rid of the caching-and-deferred-initialisation.  The
original bug was solved by /not/ constructing a packagedb at the original site
where the task was set, rather than specifically by constructing one later.
1b. It never needed to construct a packagedb at that point anyway, because you
don't need a this-pointer to set a static member; it was that misunderstanding
that caused the bug in the first place.

2.  There's no need to construct a packagedb immediately after the root dir is
set; whenever we first construct one subsequently, it'll work.

3.  For that reason the patch ought to fix the original bug in any case.

  I have to run out to the shops for half an hour now, and when I get back
I'll fire up a VM, and make absolutely certain.

    cheers,
      DaveK


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