Hello Cory, hello all, > On 14 June 2018 at 22:41 Cory Mickelson <mickelsonc...@gmail.com> wrote: > > > Please accept this patch to fix PdfPage DeleteAnnotation. I've added a
I have the following objections against accepting your new patch: 1. You have changed pItem from an address on the heap to a stack-allocated object. However, this breaks querying for any cached annotations: in your version, m_mapAnnotationsDirect is indexed with &pItem, which is an address on the stack (i.e. only valid within the same method call!), because pItem is a method-local (i.e. stack-allocated) object. This means, because m_mapAnnotationsDirect contains addresses valid for longer (IIRC all pointing into the heap), that the changed indexing will never work. Therefore, the memory the entries are pointing to will not be freed, leaking memory. > test case to PageTest, you can verify the existence of this bug by running > the test before applying the changes to PdfPage.cpp. 2. The test-case part of your patch contains very many unnecessary changes (you were warned against those by the committer zyx), namely to the formatting, which are IIRC also against project CodingStyle policy. 3. Your test-case declaration testDeleteAnnotation() can never be called because you didn't implement it (the definition is missing). Please don't add your test code to an existing test case, which you've done here with PageTest::testEmptyContentsStream(). For these reasons, I can't accept this patch. Please provide a corrected one. Thank you in advance. > > Thank you, > Cory Mickelson > Best regards, mabri ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
