Re: [PATCH] Incidental setup.exe patches #3: Simplify packagedb task handling

[Dave Korn <dave dot korn dot cygwin at googlemail dot com>]

On 14/04/2010 15:41, Dave Korn wrote:

>   I'll rewrite the comments now I know exactly what's going on and resend it
> later.

  Here:

	* root.cc (RootPage::OnNext): Don't construct a packagedb here nor
	do deferred initialisation of static packagedb::task.
	* source.cc (save_dialog): Don't construct a packagedb here, and
	set static packagedb::task directly instead of chosen_db_task.
	* package_meta.cc (packagemeta::action_caption): Don't bother to
	construct a packagedb here, just access packagedb::task directly.
	* package_db.cc: Move 'static members' comment near static members.
	(chosen_db_task): Delete.
	* package_db.h (chosen_db_task): Don't declare extern.
	(packagedb): Extend comments on class.

  OK now?

    cheers,
      DaveK

Index: root.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/root.cc,v
retrieving revision 2.25
diff -p -u -r2.25 root.cc
--- root.cc	28 Jan 2010 22:59:09 -0000	2.25
+++ root.cc	14 Apr 2010 16:46:25 -0000
@@ -287,11 +287,6 @@ RootPage::OnNext ()
     << (root_text == IDC_ROOT_TEXT ? " text" : " binary")
     << (root_scope == IDC_ROOT_USER ? " user" : " system") << endLog;
 
-  /* Deferred initialization of packagedb *after* the root dir has been
-     chosen. */
-  packagedb db;
-  db.task = chosen_db_task;
-
   return 0;
 }
 
Index: source.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/source.cc,v
retrieving revision 2.21
diff -p -u -r2.21 source.cc
--- source.cc	28 Jun 2009 03:50:43 -0000	2.21
+++ source.cc	14 Apr 2010 16:46:25 -0000
@@ -54,7 +54,12 @@ static void
 save_dialog (HWND h)
 {
   source = rbget (h, rb);
-  chosen_db_task =
+  /* We mustn't construct any packagedb objects until after the root
+     directory has been selected, but setting the static data member
+     that records the mode we're running in is fine here (and conversely,
+     would be A Bad Thing if we did it again after the first time we
+     construct a packagedb object; see package_db.h for details).  */
+  packagedb::task = 
     source == IDC_SOURCE_DOWNLOAD ? PackageDB_Download : PackageDB_Install;
 }
 
Index: package_meta.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_meta.cc,v
retrieving revision 2.56
diff -p -u -r2.56 package_meta.cc
--- package_meta.cc	13 Dec 2009 19:23:43 -0000	2.56
+++ package_meta.cc	14 Apr 2010 16:46:25 -0000
@@ -362,10 +362,7 @@ packagemeta::action_caption () const
   else if (!desired)
     return "Skip";
   else if (desired == installed && desired.picked())
-    {
-      packagedb db;
-      return db.task == PackageDB_Install ? "Reinstall" : "Retrieve";
-    }
+    return packagedb::task == PackageDB_Install ? "Reinstall" : "Retrieve";
   else if (desired == installed && desired.sourcePackage() && desired.sourcePackage().picked())
     /* FIXME: Redo source should come up if the tarball is already present locally */
     return "Source";
Index: package_db.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_db.cc,v
retrieving revision 2.43
diff -p -u -r2.43 package_db.cc
--- package_db.cc	18 Dec 2009 11:59:54 -0000	2.43
+++ package_db.cc	14 Apr 2010 16:46:25 -0000
@@ -45,10 +45,6 @@ static const char *cvsid =
 
 using namespace std;
 
-PackageDBActions chosen_db_task = PackageDB_Install;
-
-/* static members */
-
 packagedb::packagedb ()
 {
   io_stream *db = 0;
@@ -199,6 +195,8 @@ packagedb::findSource (PackageSpecificat
   return NULL;
 }
 
+/* static members */
+
 int
   packagedb::installeddbread =
   0;
Index: package_db.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/package_db.h,v
retrieving revision 2.23
diff -p -u -r2.23 package_db.h
--- package_db.h	12 Aug 2008 20:32:08 -0000	2.23
+++ package_db.h	14 Apr 2010 16:46:25 -0000
@@ -29,12 +29,33 @@ typedef enum {
   PackageDB_Download
 } PackageDBActions;
 
-extern PackageDBActions chosen_db_task;
-
 class packagedb;
 typedef std::vector <packagemeta *>::iterator PackageDBConnectedIterator;
 
 /*TODO: add mutexs */
+
+/*TODO: add sanity.   Beware, Here Be C++ Dragons:
+
+   This class is a hidden singleton.  It's a singleton, but you create
+   and delete transient objects of the class, none of which have any
+   member data, but solely serve as shortcuts to access one static set
+   of shared data through an irrelevant this-pointer.
+
+   Not only that, but it's a hidden singleton that is constructed
+   the first time you access it, and constructed differently 
+   based on implicit global state.
+
+   Not only that, but it has some static state of its own that also
+   controls how it gets constructed, but that could then be changed
+   afterward without invalidating the cached data, silently changing
+   its semantic interpretation.
+
+   To use this class, you must first set the packagedb::task member
+   and the cygfile:// (install dir) root path.  You must only then
+   construct a packagedb, and must remember not to change the
+   task or root path any later in the execution sequence.
+
+*/
 class packagedb
 {
 public: