Re: Review Request 125971: k7zip: fix memleaks, lower memory usage

[Nick Shaforostoff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125971/
---

(Updated Nov. 7, 2015, 10:07 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure and Laurent Montel.


Changes
---

Submitted with commit 5e00ad1f31a6cb436f3d17896de82e1691acfefa by Nick 
Shaforostoff to branch master.


Repository: karchive


Description
---

i couldn't find the place where the pointers contained in the member arrays are 
deleted so i have added their releasing. for this i have used qDeleteAll (you 
can search for qDeleteAll in the diff)

also i have reordered members of FileInfo to reduce its 'sizeof'
also using qvector for storing pointers is as fast as using qlist (or even 
faster) and needs less memory
also i have switched qvector acces from operator[] to .at() because it is const 
(doesn't call detach() method internally)
also i have disabled the code that was filling 'method' string because it was 
not used anywhere after


Diffs
-

  src/k7zip.cpp 321620a 
  src/karchive.cpp 0ece37c 

Diff: https://git.reviewboard.kde.org/r/125971/diff/


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125971: k7zip: fix memleaks, lower memory usage

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125971/#review88120
---

Ship it!


Looks good. Please do a "make test" before pushing, though.

- David Faure


On Nov. 7, 2015, 12:35 a.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125971/
> ---
> 
> (Updated Nov. 7, 2015, 12:35 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Laurent Montel.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> i couldn't find the place where the pointers contained in the member arrays 
> are deleted so i have added their releasing. for this i have used qDeleteAll 
> (you can search for qDeleteAll in the diff)
> 
> also i have reordered members of FileInfo to reduce its 'sizeof'
> also using qvector for storing pointers is as fast as using qlist (or even 
> faster) and needs less memory
> also i have switched qvector acces from operator[] to .at() because it is 
> const (doesn't call detach() method internally)
> also i have disabled the code that was filling 'method' string because it was 
> not used anywhere after
> 
> 
> Diffs
> -
> 
>   src/k7zip.cpp 321620a 
>   src/karchive.cpp 0ece37c 
> 
> Diff: https://git.reviewboard.kde.org/r/125971/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125971: k7zip: fix memleaks, lower memory usage

[Nick Shaforostoff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125971/
---

(Updated Nov. 6, 2015, 5:32 p.m.)


Review request for KDE Frameworks and Laurent Montel.


Changes
---

I have removed qDeleteAll(m_entryList) as it was causing one test to fail.

I have added a ~K7ZipFileEntry() destructor to delete m_buffer -> this removes 
about 1mb leaked memory during 'make test' execution, as shown by valgrind


Repository: karchive


Description
---

i couldn't find the place where the pointers contained in the member arrays are 
deleted so i have added their releasing. for this i have used qDeleteAll (you 
can search for qDeleteAll in the diff)

also i have reordered members of FileInfo to reduce its 'sizeof'
also using qvector for storing pointers is as fast as using qlist (or even 
faster) and needs less memory
also i have switched qvector acces from operator[] to .at() because it is const 
(doesn't call detach() method internally)
also i have disabled the code that was filling 'method' string because it was 
not used anywhere after


Diffs (updated)
-

  src/k7zip.cpp 321620a 
  src/karchive.cpp 0ece37c 

Diff: https://git.reviewboard.kde.org/r/125971/diff/


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125971: k7zip: fix memleaks, lower memory usage

[Milian Wolff]

> On Nov. 6, 2015, 12:25 p.m., Milian Wolff wrote:
> > I'd remove the code, instead of just commenting it out if it's really not 
> > needed.
> > 
> > Also, minor nitpick: QList is pretty much equivalent to QVector, 
> > you won't see any big difference, performance or memory wise. That said, 
> > I'm all for using QVector everywhere by default. It's certainly not worse 
> > than QList ;-)
> > 
> > Thanks!
> > 
> > Oh and maybe wait for a review of David before pushing this. I have no clue 
> > about the code itself. The patch looks fine from my POV though.

above should read:

...`QList` is pretty much equivalent to `QVector`...


- Milian


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125971/#review88094
---


On Nov. 5, 2015, 11:55 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125971/
> ---
> 
> (Updated Nov. 5, 2015, 11:55 p.m.)
> 
> 
> Review request for KDE Frameworks and Laurent Montel.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> i couldn't find the place where the pointers contained in the member arrays 
> are deleted so i have added their releasing. for this i have used qDeleteAll 
> (you can search for qDeleteAll in the diff)
> 
> also i have reordered members of FileInfo to reduce its 'sizeof'
> also using qvector for storing pointers is as fast as using qlist (or even 
> faster) and needs less memory
> also i have switched qvector acces from operator[] to .at() because it is 
> const (doesn't call detach() method internally)
> also i have disabled the code that was filling 'method' string because it was 
> not used anywhere after
> 
> 
> Diffs
> -
> 
>   src/k7zip.cpp 321620a 
> 
> Diff: https://git.reviewboard.kde.org/r/125971/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125971: k7zip: fix memleaks, lower memory usage

[Milian Wolff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125971/#review88094
---

Ship it!


I'd remove the code, instead of just commenting it out if it's really not 
needed.

Also, minor nitpick: QList is pretty much equivalent to QVector, you 
won't see any big difference, performance or memory wise. That said, I'm all 
for using QVector everywhere by default. It's certainly not worse than QList ;-)

Thanks!

Oh and maybe wait for a review of David before pushing this. I have no clue 
about the code itself. The patch looks fine from my POV though.


src/k7zip.cpp (line 2644)


this looks odd, either remove it or not, but don't just comment out the code


- Milian Wolff


On Nov. 5, 2015, 11:55 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125971/
> ---
> 
> (Updated Nov. 5, 2015, 11:55 p.m.)
> 
> 
> Review request for KDE Frameworks and Laurent Montel.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> i couldn't find the place where the pointers contained in the member arrays 
> are deleted so i have added their releasing. for this i have used qDeleteAll 
> (you can search for qDeleteAll in the diff)
> 
> also i have reordered members of FileInfo to reduce its 'sizeof'
> also using qvector for storing pointers is as fast as using qlist (or even 
> faster) and needs less memory
> also i have switched qvector acces from operator[] to .at() because it is 
> const (doesn't call detach() method internally)
> also i have disabled the code that was filling 'method' string because it was 
> not used anywhere after
> 
> 
> Diffs
> -
> 
>   src/k7zip.cpp 321620a 
> 
> Diff: https://git.reviewboard.kde.org/r/125971/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 125971: k7zip: fix memleaks, lower memory usage

[Nick Shaforostoff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125971/
---

Review request for KDE Frameworks and Laurent Montel.


Repository: karchive


Description
---

i couldn't find the place where the pointers contained in the member arrays are 
deleted so i have added their releasing. for this i have used qDeleteAll (you 
can search for qDeleteAll in the diff)

also i have reordered members of FileInfo to reduce its 'sizeof'
also using qvector for storing pointers is as fast as using qlist (or even 
faster) and needs less memory
also i have switched qvector acces from operator[] to .at() because it is const 
(doesn't call detach() method internally)
also i have disabled the code that was filling 'method' string because it was 
not used anywhere after


Diffs
-

  src/k7zip.cpp 321620a 

Diff: https://git.reviewboard.kde.org/r/125971/diff/


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel