Review Request 116747: Clean up KCompletionBox

[David Gil Oliva]

> 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.
> 
> Alex Merry wrote:
>     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...

Thank you both of you for the useful hints. With this little discussion I have learned some criteria to decide what to do now and in the future.


On March 11, 2014, 11:17 p.m., David Gil Oliva wrote:
> > Have you looked through the uses of the "un-slotted" methods? (lxr.kde.org). Maybe there's a reason for that... :/
> 
> David Gil Oliva wrote:
>     Maybe I'm totally wrong, but I can't imagine any way that a getter can be useful as a slot. 
>     
>     connect (widget, SLOT(valueChanged(), completionBox, SIGNAL(isTabHandling());
>     
>     Does it make sense?    ¿?:-/
> 
> Alex Merry wrote:
>     Non-void slots are only useful if they work as a slot (ie: have some sort of side-effect) and might be called directly or with QMetaObject::invokeMethod().  If the method is const (like a getter), there's no point having it a slot at all; if you want to be able to use it with invokeMethod, you can just make it Q_INVOKABLE.
> 
> Aleix Pol Gonzalez wrote:
>     Well, having const methods as slots doesn't make sense indeed, if it's not for exposing on the meta object system, that's why I said you could do a fast lxr. I just did, didn't find anything relevant.

Thank you for looking it up :-)


- David


-----------------------------------------------------------
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/82deffd9/attachment-0001.html>