Hello Francesco, hello all,
> On 18 January 2019 at 09:29 Francesco Pretto <cez...@gmail.com> wrote:
>
>
> On Fri, 18 Jan 2019 at 01:14, Matthew Brincke <ma...@mailbox.org> wrote:
> >
> > Hello Francesco, hello all,
> >
> > > On 14 January 2019 at 23:08 Francesco Pretto <cez...@gmail.com> wrote:
> > >
> > >
> > > From: Francesco Pretto <cez...@gmail.com>
> > > ---src/base/PdfDictionary.cpp | 17 ++++++++---------
> > > 1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > the lines your patch removes each contain an explicit or
> > implicit lookup, whereas your proposal only has one lookup
> > per method (class members are not called "functions" IIRC)
> > so it removes 5 look-ups (therefore I've changed the subject).
>
> Thank you! Yes, it's 5 lookups removed, the code was very inefficient.
>
> > More importantly, this patch seems to depend on [PATCH 4/5] in
> > this series for correctness (to avoid a memory leak) because
> > it removes a "delete" operator invocation without replacement
> > (IIRC: doesn't std::map::erase(), like std::vector::erase(),
> > only remove the item(s) from the container but doesn't free
> > their memory, for "reference" types, i.e. not primitives?).
> >
>
> Are you talking about RemoveKey() method, right? I think the
yes, I mean that one.
> delete invocation is still missing, also in [PATCH 4/5], as
> map::erase() can't release memory for pointers.
I'm sorry I didn't yet review [PATCH 4/5], thank you for finding
the mistake yourself and mentioning it here.
>
> Fixed method would be:
>
> TKeyMap::iterator found = m_mapKeys.find( identifier );
> if( found != m_mapKeys.end() )
> {
> AssertMutable();
> delete found->second;
> m_mapKeys.erase( found );
> m_bDirty = true;
> return true;
> }
Thank you for providing the fix here, I'll credit you for the
patch and the fix because I'd like to accept the first two
patches in this series today (commit them after testing with
different compilers on GNU/Linux).
>
> I'm a bit in a stale situation now. Can I fix the change in
> a bigger merged patch 1-5?
I advise against that, because for accepting the two first
patches I'd need them separate, and I don't think you need
to provide a fixed version of the second patch as the fix
is already in the post I'm replying to and it's a simple one
so I can easily insert it into the code before testing.
I'm not sure I can accept the further patches because of the
(look-like at least for me) ABI changes, before the release
coming next, I think it'd be 0.9.7 and consist of fixes for
security vulnerabilities, crashes and exceptions avoidable
without changing the ABI or API (maybe supporting more uses).
Best regards, Matthew
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
