Attachment 73732 Details for Bug 59933 – cleanup code

[patch] cleanup code

threadsafecleanup.diff (text/plain), 12.27 KB, created by Thomas Freitag on 2013-01-27 14:20:25 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Thomas Freitag

Created: 2013-01-27 14:20:25 UTC

Size: 12.27 KB

patch

obsolete

>diff --git a/goo/GooMutex.h b/goo/GooMutex.h
>index 3f53a62..fbef476 100644
>--- a/goo/GooMutex.h
>+++ b/goo/GooMutex.h
>@@ -60,4 +60,20 @@ typedef pthread_mutex_t GooMutex;
> 
> #endif
> 
>+namespace Poppler {
>+  enum LockMode {
>+    DoNotLock,  // for conditional locks: do not lock 
>+    DoLock      // for conditional locks: do lock 
>+  };
>+
>+  class Lock {
>+  public:
>+    Lock(GooMutex *mutexA) : mutex(mutexA) { mode = DoLock; gLockMutex(mutex); }
>+    Lock(GooMutex *mutexA, LockMode modeA) : mutex(mutexA) { mode = modeA; if (mode == DoLock) gLockMutex(mutex); }
>+    ~Lock() { if (mode == DoLock) gUnlockMutex(mutex); }
>+  private:
>+    GooMutex *mutex;
>+    LockMode mode;
>+  };
>+}
> #endif
>diff --git a/poppler/Annot.cc b/poppler/Annot.cc
>index 0b3c5e4..5bc645a 100644
>--- a/poppler/Annot.cc
>+++ b/poppler/Annot.cc
>@@ -1205,7 +1205,7 @@ void Annot::initialize(PDFDoc *docA, Dict *dict) {
>   if (dict->lookupNF("P", &obj1)->isRef()) {
>     Ref ref = obj1.getRef();
> 
>-    page = doc->getCatalog()->findPage (ref.num, ref.gen, gFalse);
>+    page = doc->getCatalog()->findPage (ref.num, ref.gen, Poppler::DoNotLock);
>   } else {
>     page = 0;
>   }
>@@ -3770,7 +3770,7 @@ AnnotWidget::~AnnotWidget() {
> void AnnotWidget::initialize(PDFDoc *docA, Dict *dict) {
>   Object obj1;
> 
>-  form = doc->getCatalog()->getForm(gFalse);
>+  form = doc->getCatalog()->getForm(Poppler::DoNotLock);
> 
>   if(dict->lookup("H", &obj1)->isName()) {
>     const char *modeName = obj1.getName();
>diff --git a/poppler/Array.cc b/poppler/Array.cc
>index d23ed5d..79551e6 100644
>--- a/poppler/Array.cc
>+++ b/poppler/Array.cc
>@@ -35,11 +35,9 @@
> #include "Array.h"
> 
> #if MULTITHREADED
>-#  define lockArray   gLockMutex(&mutex)
>-#  define unlockArray gUnlockMutex(&mutex)
>+#  define lockArray   Poppler::Lock lock(&mutex)
> #else
> #  define lockArray
>-#  define unlockArray
> #endif
> //------------------------------------------------------------------------
> // Array
>@@ -73,21 +71,18 @@ Object *Array::copy(XRef *xrefA, Object *obj) {
>     Object obj1;
>     obj->arrayAdd(elems[i].copy(&obj1));
>   }
>-  unlockArray;
>   return obj;
> }
> 
> int Array::incRef() {
>   lockArray;
>   ++ref;
>-  unlockArray;
>   return ref;
> }
> 
> int Array::decRef() {
>   lockArray;
>   --ref;
>-  unlockArray;
>   return ref;
> }
> 
>@@ -103,7 +98,6 @@ void Array::add(Object *elem) {
>   }
>   elems[length] = *elem;
>   ++length;
>-  unlockArray;
> }
> 
> void Array::remove(int i) {
>@@ -112,13 +106,11 @@ void Array::remove(int i) {
> #ifdef DEBUG_MEM
>     abort();
> #else
>-    unlockArray;
>     return;
> #endif
>   }
>   --length;
>   memmove( elems + i, elems + i + 1, sizeof(elems[0]) * (length - i) );
>-  unlockArray;
> }
> 
> Object *Array::get(int i, Object *obj, int recursion) {
>diff --git a/poppler/Catalog.cc b/poppler/Catalog.cc
>index b7f7ede..d4584bc 100644
>--- a/poppler/Catalog.cc
>+++ b/poppler/Catalog.cc
>@@ -57,11 +57,11 @@
> #include "FileSpec.h"
> 
> #if MULTITHREADED
>-#  define lockCatalog   gLockMutex(&mutex)
>-#  define unlockCatalog gUnlockMutex(&mutex)
>+#  define lockCatalog()   Poppler::Lock lock(&mutex)
>+#  define condLockCatalog(X)  Poppler::Lock condlock(&mutex, (X))
> #else
>-#  define lockCatalog
>-#  define unlockCatalog
>+#  define lockCatalog()
>+#  define condLockCatalog(X)
> #endif
> //------------------------------------------------------------------------
> // Catalog
>@@ -191,7 +191,7 @@ GooString *Catalog::readMetadata() {
>   Dict *dict;
>   Object obj;
> 
>-  lockCatalog;
>+  lockCatalog();
>   if (metadata.isNone()) {
>     Object catDict;
> 
>@@ -206,7 +206,6 @@ GooString *Catalog::readMetadata() {
>   }
> 
>   if (!metadata.isStream()) {
>-    unlockCatalog;
>     return NULL;
>   }
>   dict = metadata.streamGetDict();
>@@ -218,7 +217,6 @@ GooString *Catalog::readMetadata() {
>   s = new GooString();
>   metadata.getStream()->fillGooString(s);
>   metadata.streamClose();
>-  unlockCatalog;
>   return s;
> }
> 
>@@ -226,31 +224,27 @@ Page *Catalog::getPage(int i)
> {
>   if (i < 1) return NULL;
> 
>-  lockCatalog;
>+  lockCatalog();
>   if (i > lastCachedPage) {
>      GBool cached = cachePageTree(i);
>      if ( cached == gFalse) {
>-       unlockCatalog;
>        return NULL;
>      }
>   }
>-  unlockCatalog;
>   return pages[i-1];
> }
> 
>-Ref *Catalog::getPageRef(int i, GBool lock)
>+Ref *Catalog::getPageRef(int i, Poppler::LockMode lock)
> {
>   if (i < 1) return NULL;
> 
>-  if (lock) lockCatalog;
>+  condLockCatalog(lock);
>   if (i > lastCachedPage) {
>      GBool cached = cachePageTree(i);
>      if ( cached == gFalse) {
>-       if (lock) unlockCatalog;
>        return NULL;
>      }
>   }
>-  if (lock) unlockCatalog;
>   return &pageRefs[i-1];
> }
> 
>@@ -300,7 +294,7 @@ GBool Catalog::cachePageTree(int page)
>       return gFalse;
>     }
> 
>-    pagesSize = getNumPages(gFalse);
>+    pagesSize = getNumPages(Poppler::DoLock);
>     pages = (Page **)gmallocn(pagesSize, sizeof(Page *));
>     pageRefs = (Ref *)gmallocn(pagesSize, sizeof(Ref));
>     for (int i = 0; i < pagesSize; ++i) {
>@@ -427,7 +421,7 @@ GBool Catalog::cachePageTree(int page)
>   return gFalse;
> }
> 
>-int Catalog::findPage(int num, int gen, GBool lock) {
>+int Catalog::findPage(int num, int gen, Poppler::LockMode lock) {
>   int i;
> 
>   for (i = 0; i < getNumPages(lock); ++i) {
>@@ -452,12 +446,11 @@ LinkDest *Catalog::findDest(GooString *name) {
>       obj1.free();
>   }
>   if (!found) {
>-    lockCatalog;
>+    lockCatalog();
>     if (getDestNameTree()->lookup(name, &obj1))
>       found = gTrue;
>     else
>       obj1.free();
>-    unlockCatalog;
>   }
>   if (!found)
>     return NULL;
>@@ -488,7 +481,7 @@ FileSpec *Catalog::embeddedFile(int i)
> {
>     Object efDict;
>     Object obj;
>-    lockCatalog;
>+    lockCatalog();
>     obj = getEmbeddedFileNameTree()->getValue(i);
>     FileSpec *embeddedFile = 0;
>     if (obj.isRef()) {
>@@ -501,7 +494,6 @@ FileSpec *Catalog::embeddedFile(int i)
>       Object null;
>       embeddedFile = new FileSpec(&null);
>     }
>-    unlockCatalog;
>     return embeddedFile;
> }
> 
>@@ -510,12 +502,11 @@ GooString *Catalog::getJS(int i)
>   Object obj;
>   // getJSNameTree()->getValue(i) returns a shallow copy of the object so we
>   // do not need to free it
>-  lockCatalog;
>+  lockCatalog();
>   getJSNameTree()->getValue(i).fetch(xref, &obj);
> 
>   if (!obj.isDict()) {
>     obj.free();
>-    unlockCatalog;
>     return 0;
>   }
>   Object obj2;
>@@ -527,7 +518,6 @@ GooString *Catalog::getJS(int i)
>   if (strcmp(obj2.getName(), "JavaScript")) {
>     obj2.free();
>     obj.free();
>-    unlockCatalog;
>     return 0;
>   }
>   obj2.free();
>@@ -543,13 +533,12 @@ GooString *Catalog::getJS(int i)
>   }
>   obj2.free();
>   obj.free();
>-  unlockCatalog;
>   return js;
> }
> 
> Catalog::PageMode Catalog::getPageMode() {
> 
>-  lockCatalog;
>+  lockCatalog();
>   if (pageMode == pageModeNull) {
> 
>     Object catDict, obj;
>@@ -560,7 +549,6 @@ Catalog::PageMode Catalog::getPageMode() {
>     if (!catDict.isDict()) {
>       error(errSyntaxError, -1, "Catalog object is wrong type ({0:s})", catDict.getTypeName());
>       catDict.free();
>-      unlockCatalog;
>       return pageMode;
>     }
> 
>@@ -581,13 +569,12 @@ Catalog::PageMode Catalog::getPageMode() {
>     obj.free();
>     catDict.free();
>   }
>-  unlockCatalog;
>   return pageMode;
> }
> 
> Catalog::PageLayout Catalog::getPageLayout() {
> 
>-  lockCatalog;
>+  lockCatalog();
>   if (pageLayout == pageLayoutNull) {
> 
>     Object catDict, obj;
>@@ -598,7 +585,6 @@ Catalog::PageLayout Catalog::getPageLayout() {
>     if (!catDict.isDict()) {
>       error(errSyntaxError, -1, "Catalog object is wrong type ({0:s})", catDict.getTypeName());
>       catDict.free();
>-      unlockCatalog;
>       return pageLayout;
>     }
> 
>@@ -620,7 +606,6 @@ Catalog::PageLayout Catalog::getPageLayout() {
>     obj.free();
>     catDict.free();
>   }
>-  unlockCatalog;
>   return pageLayout;
> }
> 
>@@ -787,13 +772,9 @@ GBool Catalog::indexToLabel(int index, GooString *label)
>   }
> }
> 
>-int Catalog::getNumPages(GBool lock)
>+int Catalog::getNumPages(Poppler::LockMode lock)
> {
>-  GBool locked = gFalse;
>-  if (lock && numPages == -1) {
>-    locked = gTrue;
>-    lockCatalog;
>-  }
>+  condLockCatalog((numPages == -1 && lock == Poppler::DoLock) ? lock : Poppler::DoNotLock);
>   if (numPages == -1)
>   {
>     Object catDict, pagesDict, obj;
>@@ -802,7 +783,6 @@ int Catalog::getNumPages(GBool lock)
>     if (!catDict.isDict()) {
>       error(errSyntaxError, -1, "Catalog object is wrong type ({0:s})", catDict.getTypeName());
>       catDict.free();
>-      if (locked) unlockCatalog;
>       return 0;
>     }
>     catDict.dictLookup("Pages", &pagesDict);
>@@ -814,7 +794,6 @@ int Catalog::getNumPages(GBool lock)
>       error(errSyntaxError, -1, "Top-level pages object is wrong type ({0:s})",
>           pagesDict.getTypeName());
>       pagesDict.free();
>-      if (locked) unlockCatalog;
>       return 0;
>     }
> 
>@@ -832,13 +811,12 @@ int Catalog::getNumPages(GBool lock)
>     pagesDict.free();
>   }
> 
>-  if (locked) unlockCatalog;
>   return numPages;
> }
> 
> PageLabelInfo *Catalog::getPageLabelInfo()
> {
>-  lockCatalog;
>+  lockCatalog();
>   if (!pageLabelInfo) {
>     Object catDict;
>     Object obj;
>@@ -847,24 +825,22 @@ PageLabelInfo *Catalog::getPageLabelInfo()
>     if (!catDict.isDict()) {
>       error(errSyntaxError, -1, "Catalog object is wrong type ({0:s})", catDict.getTypeName());
>       catDict.free();
>-      unlockCatalog;
>       return NULL;
>     }
> 
>     if (catDict.dictLookup("PageLabels", &obj)->isDict()) {
>-      pageLabelInfo = new PageLabelInfo(&obj, getNumPages(gFalse));
>+      pageLabelInfo = new PageLabelInfo(&obj, getNumPages(Poppler::DoLock));
>     }
>     obj.free();
>     catDict.free();
>   }
> 
>-  unlockCatalog;
>   return pageLabelInfo;
> }
> 
> Object *Catalog::getStructTreeRoot()
> {
>-  lockCatalog;
>+  lockCatalog();
>   if (structTreeRoot.isNone())
>   {
>      Object catDict;
>@@ -879,13 +855,12 @@ Object *Catalog::getStructTreeRoot()
>      catDict.free();
>   }
> 
>-  unlockCatalog;
>   return &structTreeRoot;
> }
> 
> Object *Catalog::getOutline()
> {
>-  lockCatalog;
>+  lockCatalog();
>   if (outline.isNone())
>   {
>      Object catDict;
>@@ -900,13 +875,12 @@ Object *Catalog::getOutline()
>      catDict.free();
>   }
> 
>-  unlockCatalog;
>   return &outline;
> }
> 
> Object *Catalog::getDests()
> {
>-  lockCatalog;
>+  lockCatalog();
>   if (dests.isNone())
>   {
>      Object catDict;
>@@ -921,7 +895,6 @@ Object *Catalog::getDests()
>      catDict.free();
>   }
> 
>-  unlockCatalog;
>   return &dests;
> }
> 
>@@ -943,9 +916,9 @@ Catalog::FormType Catalog::getFormType()
>   return res;
> }
> 
>-Form *Catalog::getForm(GBool lock)
>+Form *Catalog::getForm(Poppler::LockMode lock)
> {
>-  if (lock) lockCatalog;
>+  condLockCatalog(lock);
>   if (!form) {
>     if (acroForm.isDict()) {
>       form = new Form(doc, &acroForm);
>@@ -954,20 +927,18 @@ Form *Catalog::getForm(GBool lock)
>     }
>   }
> 
>-  if (lock) unlockCatalog;
>   return form;
> }
> 
> ViewerPreferences *Catalog::getViewerPreferences()
> {
>-  lockCatalog;
>+  lockCatalog();
>   if (!viewerPrefs) {
>     if (viewerPreferences.isDict()) {
>       viewerPrefs = new ViewerPreferences(viewerPreferences.getDict());
>     }
>   }
> 
>-  unlockCatalog;
>   return viewerPrefs;
> }
> 
>diff --git a/poppler/Catalog.h b/poppler/Catalog.h
>index ac7505f..0ddc30a 100644
>--- a/poppler/Catalog.h
>+++ b/poppler/Catalog.h
>@@ -107,13 +107,13 @@ public:
>   GBool isOk() { return ok; }
> 
>   // Get number of pages.
>-  int getNumPages(GBool lock = gTrue);
>+  int getNumPages(Poppler::LockMode lock = Poppler::DoLock);
> 
>   // Get a page.
>   Page *getPage(int i);
> 
>   // Get the reference for a page object.
>-  Ref *getPageRef(int i, GBool lock = gTrue);
>+  Ref *getPageRef(int i, Poppler::LockMode lock = Poppler::DoLock);
> 
>   // Return base URI, or NULL if none.
>   GooString *getBaseURI() { return baseURI; }
>@@ -127,7 +127,7 @@ public:
> 
>   // Find a page, given its object ID.  Returns page number, or 0 if
>   // not found.
>-  int findPage(int num, int gen, GBool lock = gTrue);
>+  int findPage(int num, int gen, Poppler::LockMode lock = Poppler::DoLock);
> 
>   // Find a named destination.  Returns the link destination, or
>   // NULL if <name> is not a destination.
>@@ -165,7 +165,7 @@ public:
>   };
> 
>   FormType getFormType();
>-  Form* getForm(GBool lock = gTrue);
>+  Form* getForm(Poppler::LockMode lock = Poppler::DoLock);
> 
>   ViewerPreferences *getViewerPreferences();
> 
>@@ -228,7 +228,6 @@ private:
>   PageMode pageMode;		// page mode
>   PageLayout pageLayout;	// page layout
> 
>-  void createPages();       // create pages for caching
>   GBool cachePageTree(int page); // Cache first <page> pages.
>   Object *findDestInTree(Object *tree, GooString *name, Object *obj);
> 
>

Actions: View | Splinter Review

Attachments on bug 59933: 73732 | 75446 | 75854

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.