Review Request 120075: KArchive: add support for rcc files

[Milian Wolff]

> On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote:
> > src/krcc.h, line 93
> > <https://git.reviewboard.kde.org/r/120075/diff/1/?file=310011#file310011line93>
> >
> >     scoped pointer?
> 
> David Faure wrote:
>     Hmm. Not done anywhere else in karchive or frameworks in general. Not much point, now that I checked the "delete" is there :-)

You should start using smart pointers, and aim for the goal to never write a manual delete and new eventually.


> On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote:
> > src/krcc.cpp, line 154
> > <https://git.reviewboard.kde.org/r/120075/diff/1/?file=310012#file310012line154>
> >
> >     maybe I'm missing something, but this looks like you don't need to override it at all when you just delegate to the base class anyways?
> 
> David Faure wrote:
>     The whole point is to be able to extend this later without requiring an app recompilation.

as I said, maybe I'm missing something, but here I come again:

the base function is virtual, so it will always go through the vtable, no matter whether you added an overload or not. you'll only ever need to recompile karchive when you add functionality to it, and that you'll need to do no matter what since you'll need to add code to this function in such a case.


- Milian


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


On Oct. 11, 2014, 3:37 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120075/
> -----------------------------------------------------------
> 
> (Updated Oct. 11, 2014, 3:37 p.m.)
> 
> 
> Review request for KDE Frameworks, Mario Bensi and Kevin Krammer.
> 
> 
> Repository: karchive
> 
> 
> Description
> -------
> 
> These the dynamic binary resources created by Qt's rcc tool from a .qrc file
> and the files it points to.
> 
> [ChangeLog] KArchive: add support for rcc files
> 
> REVIEW: 120075
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 
>   src/krcc.h PRE-CREATION 
>   src/krcc.cpp PRE-CREATION 
>   tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 
>   tests/krcctest.cpp PRE-CREATION 
>   autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb 
>   autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 
>   autotests/runtime_resource.rcc PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/120075/diff/
> 
> 
> Testing
> -------
> 
> Manual test prog, autotest.
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20141011/c5f2fa90/attachment-0001.html>