Created attachment 73732 [details] [review] cleanup code This is a first try to cleanup the code for patch for bug 50992. In this first step I just did it for Array.cc/.h and Catalog.cc/.h. If it's okay for You, should I continue doing it per c++ file or do You want a patch for all?
Comment on attachment 73732 [details] [review] cleanup code >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); > >
Looks good to me, i think it makes the code more readable. I think it'd make more sense all the files at once, what do you think?
Created attachment 75446 [details] [review] cleanup code Here now the complete cleanup patch. Even if successfully regtested, please have again a deeper look into the code.
(In reply to comment #3) > Created attachment 75446 [details] [review] [review] > cleanup code > > Here now the complete cleanup patch. Even if successfully regtested, please > have again a deeper look into the code. Just a short remark on the "Lock" class: I think you could merge the two constructors using a default argument, e.g. Lock(GooMutex *mutexA, LockMode modeA = DoLock) : mutex(mutexA) { mode = modeA; if (mode == DoLock) gLockMutex(mutex); } and comparing it with Qt's QMutexLocker, explicit "lock" and "unlock" methods might be useful if one wants to break RAII locally.
(In reply to comment #4) > (In reply to comment #3) > > Created attachment 75446 [details] [review] [review] [review] > > cleanup code > > > > Here now the complete cleanup patch. Even if successfully regtested, please > > have again a deeper look into the code. > > Just a short remark on the "Lock" class: I think you could merge the two > constructors using a default argument, e.g. > > Lock(GooMutex *mutexA, LockMode modeA = DoLock) : mutex(mutexA) { mode = > modeA; if (mode == DoLock) gLockMutex(mutex); } > > and comparing it with Qt's QMutexLocker, explicit "lock" and "unlock" > methods might be useful if one wants to break RAII locally. Can You take over and implement it based on my patch or do it in an additional step if Albert agrees? Reason for my question: I agree to Your comment, but I'm working such a long time and too much hours on this feature that I'm getting too tired to spend any unnecessary additional hour :-(
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Created attachment 75446 [details] [review] [review] [review] [review] > > > cleanup code > > > > > > Here now the complete cleanup patch. Even if successfully regtested, please > > > have again a deeper look into the code. > > > > Just a short remark on the "Lock" class: I think you could merge the two > > constructors using a default argument, e.g. > > > > Lock(GooMutex *mutexA, LockMode modeA = DoLock) : mutex(mutexA) { mode = > > modeA; if (mode == DoLock) gLockMutex(mutex); } > > > > and comparing it with Qt's QMutexLocker, explicit "lock" and "unlock" > > methods might be useful if one wants to break RAII locally. > > Can You take over and implement it based on my patch or do it in an > additional step if Albert agrees? Reason for my question: I agree to Your > comment, but I'm working such a long time and too much hours on this feature > that I'm getting too tired to spend any unnecessary additional hour :-( Of course, I'll wait till this is merged and propose a small patch afterwards. Thanks for all the hard work!
Created attachment 75854 [details] [review] Patch on top of Thomas' I'm attaching a patch that i want to commit on top of Thomas' it has a few changes: * Remove the namespace (we don't use much/any namespaces in poppler core) * Rename the class and defines from lock to locker since lock and be either the action "to lock" or the "thing that locks", with locker it is more clear (i think) that is "the thing" than "the action" * Make Annot::decRefCnt use gLockMutex since we the object itself is being deleted in the if and not sure the locker would be happy with that * change the getNumPages() param to be DoNotLockMutex since previously it was a gFalse (i guess Thomas made a c&p typo here) * Have only one constructor like Adam suggested. Comments?
Hello Albert, (In reply to comment #7) > Created attachment 75854 [details] [review] [review] > Patch on top of Thomas' > > I'm attaching a patch that i want to commit on top of Thomas' it has a few > changes: > * Remove the namespace (we don't use much/any namespaces in poppler core) Actually, I had a few stray crashes in qpdfview because of pure virtual methods being called through naming collisions with Poppler internal (e.g. defining an ::Outline class), but of course, namespacing the Locker won't help that. :-) > * Rename the class and defines from lock to locker since lock and be either > the action "to lock" or the "thing that locks", with locker it is more clear > (i think) that is "the thing" than "the action" I agree, it is clearer. > * Make Annot::decRefCnt use gLockMutex since we the object itself is being > deleted in the if and not sure the locker would be happy with that Makes sense. > * change the getNumPages() param to be DoNotLockMutex since previously it > was a gFalse (i guess Thomas made a c&p typo here) > * Have only one constructor like Adam suggested. > > Comments? Best regards, Adam.
Ok from my side
Commited
If anyone has more improvements please open a new bug so it has a smaller "log"
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.