unifying libdiff2
Jeremy Whiting
jpwhiting at kde.org
Fri Oct 28 03:33:47 UTC 2011
On Thu, Oct 27, 2011 at 8:46 PM, Kevin Kofler <kevin.kofler at chello.at> wrote:
> On Friday 28 October 2011, Jeremy Whiting wrote:
>> That said in comparing the two versions I merged some things locally
>> from kdevelop version to kdesdk version. I've attached a patch to
>> kdesdk version with those improvements. I suggest these changes get
>> applied to kdesdk version and after the dependency issue is resolved
>> the forked version go away.
>
> Reviewing those changes:
>
> * Removing the endl after kDebug is an obvious bugfix, so obviously OK.
>
> * Re these komparemodellist.cpp changes:
> -
> - if( !temp.open() ) {
> + bool correct=temp.open();
> +
> + if( !correct ) {
> and:
> + bool opened=m_diffTemp->open();
>
> - if( !m_diffTemp->open() ) {
> + if( !opened ) {
> Why the extra variables? In fact, I removed those variables when I fixed the
> inverted login in revision 1145313 because they're redundant. They clearly
> didn't help preventing inverted logic. ;-)
>
> * Moving the operator == on Marker inside the class is OK. (I don't think
> there was anything wrong per se with the non-member version, but making it a
> member is more common.)
>
> * Removing spaces from the blank lines in kompareprocess is of course OK. (I
> hate spaces in blank lines!)
>
> * The optimization of begin and end to constBegin and constEnd in
> kompareprocess is also OK. That said, while we are at optimizing things,
> shouldn't constEnd also be saved to a variable? As the code stands now, we're
> calling constEnd() at every loop iteration.
>
> Kevin Kofler
>
Ok, took out the komparemodellist.cpp changes. Cached the constEnd()
and committed.
thanks,
Jeremy
More information about the KDevelop-devel
mailing list