Review Request 116747: Clean up KCompletionBox

[Alex Merry]

> On March 11, 2014, 11:17 p.m., Aleix Pol Gonzalez wrote:
> > src/kcompletionbox.h, line 228
> > <https://git.reviewboard.kde.org/r/116747/diff/1/?file=253404#file253404line228>
> >
> >     I wouldn't leave the implementation here. Move it to the .cpp file, this way it can be changed in the future, if it's required for some reason.
> >     
> >     Also there's a typo in the method name.
> 
> David Gil Oliva wrote:
>     Alex Merry inlined deprecated methods in https://git.reviewboard.kde.org/r/116012 , so I thought that it was the way to go...
> 
> Alex Merry wrote:
>     Well, there's a balance to be struck: putting them in the header ensures there is no runtime cost to programs that don't use the deprecated methods, as the code should be optimised away, that the library is always binary-compatible even if you compile it with deprecated code disabled (*_NO_DEPRECATED) and makes the header code document how to replace existing calls.  The downsides are an inability to fix the code later and an inability to access members of a private d-pointer class.  Neither of those are an issue here, as we're just renaming the method.
>     
>     tl;dr: I disagree with Aleix, and think it should stay in the header.
>     
>     Oh, and Aleix: could you please select the whole method when you're doing a comment like this, rather than just the first line? Otherwise it's a pain to see what you're referring to.  Thanks :-)
> 
> Aleix Pol Gonzalez wrote:
>     Well, I wouldn't bother about runtime penalty given that it's deprecated and we shouldn't be using it anyway. Also we can't make assumptions on how things are going to be optimized.
>     
>     But it's ok, I don't think it's worth discussing further, I doubt this is going to be a problem in the future.

I mean the runtime penalty for things that *don't* use it.  If it's header-only, it doesn't go in the library, so there is no load-time symbol-lookup penalty, and no code-size penalty.  Admittedly, both of those are probably negligible...


- Alex


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


On March 11, 2014, 10:32 p.m., David Gil Oliva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116747/
> -----------------------------------------------------------
> 
> (Updated March 11, 2014, 10:32 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> -------
> 
> Clean up KCompletionBox
> 
> -canceled() -> cancelled (private method)
> -Deprecate sizeAndPosition() --> resizeAndReposition()
> -Remove old comments and commented-out code
> -Move some slots to be normal methods, since they don't seem to be able to
> work as slots.
> 
> 
> Diffs
> -----
> 
>   src/kcompletionbox.h 09b7527 
>   src/kcompletionbox.cpp 92e87b3 
> 
> Diff: https://git.reviewboard.kde.org/r/116747/diff/
> 
> 
> Testing
> -------
> 
> It builds and tests pass.
> 
> 
> Thanks,
> 
> David Gil Oliva
> 
>

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