Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] -[core/vcl/source/window/splitwin.cxx:2045]:(style) Found duplicate branches for if and else.
On 02/18/2012 03:21 PM, julien2412 wrote: This subject, switch/if else, reminded me what I read recently on the Stroustrup book. 1) The fact that switch allows a better quality machine code. I wouldn't bet anything on that one. Compilers should be able to automatically identify if-else-chains that can be turned into switch-like code. 2) an enumeration used with a switch allows a compiler to detect a case which has be forgotten. That's indeed a reason it might be appropriate to go with a switch statement in certain cases. Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] -[core/vcl/source/window/splitwin.cxx:2045]:(style) Found duplicate branches for if and else.
I have a 'replacement' patch that actually replaces most of if...else if...else chains with switches. They are more legible and seem natural when choosing branches depending on the same variable. The performance of a switch should also be better, but turbo boost should not be expected ;). Two declarations of unused variable removed by-the-way. Anyway, all started with cppcheck warning about duplicate branches for if-else. Question I have, is how do I submit the patch: - as a reply to this thread, or - a new patch in a new thread? Regards, Mariusz Dykierek ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] -[core/vcl/source/window/splitwin.cxx:2045]:(style) Found duplicate branches for if and else.
On 18.02.2012 17:29, Mariusz Dykierek wrote: I have a 'replacement' patch that actually replaces most of if...else if...else chains with switches. Great! They are more legible and seem natural when choosing branches depending on the same variable. On the other hand, the evaluated expression is not visible at the first-look... A lot of people - a lot of opinions :) Question I have, is how do I submit the patch: - as a reply to this thread, or - a new patch in a new thread? I think, if a patch extends or replaces a current one - submit as a reply, otherwise - start a new thread. Cheers, Ivan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] -[core/vcl/source/window/splitwin.cxx:2045]:(style) Found duplicate branches for if and else.
On 2012-02-18 14:41, Ivan Timofeev wrote:
On 18.02.2012 17:29, Mariusz Dykierek wrote:
I have a 'replacement' patch that actually replaces most of if...else
if...else chains with
switches.
Great!
They are more legible and seem natural when choosing branches depending on
the same variable.
On the other hand, the evaluated expression is not visible at the
first-look... A lot of
people - a lot of opinions :)
Question I have, is how do I submit the patch:
- as a reply to this thread, or
- a new patch in a new thread?
I think, if a patch extends or replaces a current one - submit as a reply,
otherwise - start a
new thread.
Cheers,
Ivan
Attached is the new patch that replaces and extends the previous one in this
thread.
Regards,
Mariusz
From 66820932b045797e12bd49996fe5f60d5c4ef280 Mon Sep 17 00:00:00 2001
From: Mariusz Dykierek mariuszdykie...@gmail.com
Date: Sat, 18 Feb 2012 14:18:19 +0100
Subject: [PATCH] Replaced 'if-elseif-else' chains with 'switches' where they
seem natural
---
vcl/source/window/splitwin.cxx | 218
1 files changed, 109 insertions(+), 109 deletions(-)
diff --git a/vcl/source/window/splitwin.cxx b/vcl/source/window/splitwin.cxx
index f5da206..dfe66d1 100644
--- a/vcl/source/window/splitwin.cxx
+++ b/vcl/source/window/splitwin.cxx
@@ -135,33 +135,32 @@ static void ImplCalcBorder( WindowAlign eAlign, sal_Bool bNoAlign,
}
else
{
-if ( eAlign == WINDOWALIGN_TOP )
+switch ( eAlign )
{
+case WINDOWALIGN_TOP:
rLeft = 2;
rTop= 2;
rRight = 2;
rBottom = 0;
-}
-else if ( eAlign == WINDOWALIGN_LEFT )
-{
+break;
+case WINDOWALIGN_LEFT:
rLeft = 2;
rTop= 2;
rRight = 0;
rBottom = 2;
-}
-else if ( eAlign == WINDOWALIGN_BOTTOM )
-{
+break;
+case WINDOWALIGN_BOTTOM:
rLeft = 2;
rTop= 0;
rRight = 2;
rBottom = 2;
-}
-else
-{
+break;
+default:
rLeft = 0;
rTop= 2;
rRight = 2;
rBottom = 2;
+break;
}
}
}
@@ -183,8 +182,9 @@ void SplitWindow::ImplDrawBorder( SplitWindow* pWin )
}
else
{
-if ( pWin-meAlign == WINDOWALIGN_BOTTOM )
+switch ( pWin-meAlign )
{
+case WINDOWALIGN_BOTTOM:
pWin-SetLineColor( rStyleSettings.GetShadowColor() );
pWin-DrawLine( Point( 0, nDY-2 ), Point( nDX-1, nDY-2 ) );
pWin-DrawLine( Point( 0, 0 ), Point( 0, nDY-1 ) );
@@ -194,9 +194,8 @@ void SplitWindow::ImplDrawBorder( SplitWindow* pWin )
pWin-DrawLine( Point( 0, nDY-1 ), Point( nDX-1, nDY-1 ) );
pWin-DrawLine( Point( 1, 1 ), Point( 1, nDY-3 ) );
pWin-DrawLine( Point( nDX-1, 0 ), Point( nDX-1, nDY-1 ) );
-}
-else if ( pWin-meAlign == WINDOWALIGN_TOP )
-{
+break;
+case WINDOWALIGN_TOP:
pWin-SetLineColor( rStyleSettings.GetShadowColor() );
pWin-DrawLine( Point( 0, 0 ), Point( nDX-1, 0 ) );
pWin-DrawLine( Point( 0, 0 ), Point( 0, nDY-1 ) );
@@ -206,9 +205,8 @@ void SplitWindow::ImplDrawBorder( SplitWindow* pWin )
pWin-DrawLine( Point( 1, 1 ), Point( nDX-3, 1 ) );
pWin-DrawLine( Point( 1, 1 ), Point( 1, nDY-1 ) );
pWin-DrawLine( Point( nDX-1, 1 ), Point( nDX-1, nDY-1 ) );
-}
-else if ( pWin-meAlign == WINDOWALIGN_LEFT )
-{
+break;
+case WINDOWALIGN_LEFT:
pWin-SetLineColor( rStyleSettings.GetShadowColor() );
pWin-DrawLine( Point( 0, 0 ), Point( nDX-1, 0 ) );
pWin-DrawLine( Point( 0, 0 ), Point( 0, nDY-1 ) );
@@ -218,9 +216,8 @@ void SplitWindow::ImplDrawBorder( SplitWindow* pWin )
pWin-DrawLine( Point( 1, 1 ), Point( nDX-1, 1 ) );
pWin-DrawLine( Point( 1, 1 ), Point( 1, nDY-3 ) );
pWin-DrawLine( Point( 1, nDY-1 ), Point( nDX-1, nDY-1 ) );
-}
-else
-{
+break;
+default:
pWin-SetLineColor( rStyleSettings.GetShadowColor() );
pWin-DrawLine( Point( 0, 0 ), Point( nDX-2, 0 ) );
pWin-DrawLine( Point( nDX-2, 0 ), Point( nDX-2, nDY-3 ) );
@@ -230,6 +227,7 @@ void SplitWindow::ImplDrawBorder( SplitWindow* pWin )
pWin-DrawLine( Point( 0, 1 ), Point( nDX-3, 1 ) );
pWin-DrawLine( Point( nDX-1, 0 ), Point( nDX-1, nDY-1 ) );
pWin-DrawLine( Point( 0, nDY-1 ), Point( nDX-1, nDY-1 ) );
+break;
}
}
}
@@ -244,33 +242,32 @@ void SplitWindow::ImplDrawBorderLine( SplitWindow* pWin )
longnDX = pWin-mnDX;
long
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] -[core/vcl/source/window/splitwin.cxx:2045]:(style) Found duplicate branches for if and else.
This subject, switch/if else, reminded me what I read recently on the Stroustrup book. 1) The fact that switch allows a better quality machine code. 2) an enumeration used with a switch allows a compiler to detect a case which has be forgotten. Now, I'm not a senior C++ dev at all, just a beginner who tries to improve his knowledge so ... :-) Julien. -- View this message in context: http://nabble.documentfoundation.org/PATCH-core-vcl-source-window-splitwin-cxx-2047-core-vcl-source-window-splitwin-cxx-2045-style-Found--tp3748733p3756391.html Sent from the Dev mailing list archive at Nabble.com. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] - [core/vcl/source/window/splitwin.cxx:2045]: (style) Found duplicate branches for if and else.
Il 16/02/2012 00:02, Mariusz Dykierek ha scritto:
+ some simplification
+sal_Bool bLeft = sal_True;
+if ( ( meAlign == WINDOWALIGN_TOP ) || ( meAlign ==
WINDOWALIGN_LEFT ) )
+bLeft = sal_False;
I think that keeping possible values explicit could be a good thing so
what about a switch?
sal_Bool bLeft;
switch (meAlign) {
case WINDOWALIGN_TOP:
case WINDOWALIGN_LEFT:
bLeft = sal_False;
case WINDOWALIGN_BOTTOM:
case WINDOWALIGN_RIGHT:
default:
bLeft = sal_True;
}
Otherwise you can simplify it even more:
sal_Bool bLeft = (meAlign == WINDOWALIGN_TOP || meAlign ==
WINDOWALIGN_LEFT) ? sal_False : sal_True;
cheers
--
Riccardo Magliocchetti
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] - [core/vcl/source/window/splitwin.cxx:2045]: (style) Found duplicate branches for if and else.
On 02/16/2012 09:35 AM, Riccardo Magliocchetti wrote: Otherwise you can simplify it even more: sal_Bool bLeft = (meAlign == WINDOWALIGN_TOP || meAlign == WINDOWALIGN_LEFT) ? sal_False : sal_True; ... which of course reduces to bool bLeft = !(meAlign == WINDOWALIGN_TOP || meAlign == WINDOWALIGN_LEFT); or bool bLeft = meAlign == WINDOWALIGN_RIGHT || meAlign == WINDOWALIGN_BOTTOM given that WindowAlign has exactly those four members (and it makes the name bLeft look odd...). Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] - [core/vcl/source/window/splitwin.cxx:2045]: (style) Found duplicate branches for if and else.
On 2012-02-16 10:02, Stephan Bergmann wrote: On 02/16/2012 09:35 AM, Riccardo Magliocchetti wrote: Otherwise you can simplify it even more: sal_Bool bLeft = (meAlign == WINDOWALIGN_TOP || meAlign == WINDOWALIGN_LEFT) ? sal_False : sal_True; ... which of course reduces to bool bLeft = !(meAlign == WINDOWALIGN_TOP || meAlign == WINDOWALIGN_LEFT); or bool bLeft = meAlign == WINDOWALIGN_RIGHT || meAlign == WINDOWALIGN_BOTTOM given that WindowAlign has exactly those four members (and it makes the name bLeft look odd...). I personally find 'if' more legible than ?: and definitely expressions like b = x==y || x==z; I am not sure if WindowAlign will always have only these 4 members and possibly the author of the original version wasn't either (thus final else). Since I am a newbie here, I would vote for a simple 'if' or a 'switch'. Let me know what is the decision and I will change the code accordingly or... feel free to change the code and provide an alternative patch. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] -[core/vcl/source/window/splitwin.cxx:2045]:(style) Found duplicate branches for if and else.
Hi all, -- Mariusz Dykierek mariuszdykie...@gmail.com wrote: (16/02/2012 11:46) On 2012-02-16 10:02, Stephan Bergmann wrote: On 02/16/2012 09:35 AM, Riccardo Magliocchetti wrote: Otherwise you can simplify it even more: sal_Bool bLeft = (meAlign == WINDOWALIGN_TOP || meAlign == WINDOWALIGN_LEFT) ? sal_False : sal_True; ... which of course reduces to bool bLeft = !(meAlign == WINDOWALIGN_TOP || meAlign == WINDOWALIGN_LEFT); or bool bLeft = meAlign == WINDOWALIGN_RIGHT || meAlign == WINDOWALIGN_BOTTOM given that WindowAlign has exactly those four members (and it makes the name bLeft look odd...). I personally find 'if' more legible than ?: and definitely expressions like b = x==y || x==z; I am not sure if WindowAlign will always have only these 4 members and possibly the author of the original version wasn't either (thus final else). Since I am a newbie here, I would vote for a simple 'if' or a 'switch'. Let me know what is the decision and I will change the code accordingly or... feel free to change the code and provide an alternative patch. +1 for switch/break approach! More legible and open to possible later modifications without requiring code decodification efforts. Hopefully the compiler is smart enough to generate efficient code for such simple actions. Best regards Matteo ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH] [core/vcl/source/window/splitwin.cxx:2047] -[core/vcl/source/window/splitwin.cxx:2045]:(style) Found duplicate branches for if and else.
On 02/16/2012 12:18 PM, Matteo Casalin wrote: I personally find 'if' more legible than ?: and definitely expressions like b = x==y || x==z; I am not sure if WindowAlign will always have only these 4 members and possibly the author of the original version wasn't either (thus final else). Since I am a newbie here, I would vote for a simple 'if' or a 'switch'. Let me know what is the decision and I will change the code accordingly or... feel free to change the code and provide an alternative patch. +1 for switch/break approach! More legible and open to possible later modifications without requiring code decodification efforts. Hopefully the compiler is smart enough to generate efficient code for such simple actions. Whatever somebody chooses to actually put in here is fine with me anyway. (I only woke up when I saw the ... ? false : true construct, which is unnecessarily complex.) Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[PATCH] [core/vcl/source/window/splitwin.cxx:2047] - [core/vcl/source/window/splitwin.cxx:2045]: (style) Found duplicate branches for if and else.
+ some simplification -- Regards, Mariusz Dykerek diff --git a/vcl/source/window/splitwin.cxx b/vcl/source/window/splitwin.cxx index f5da206..09ad468 100644 --- a/vcl/source/window/splitwin.cxx +++ b/vcl/source/window/splitwin.cxx @@ -2035,17 +2035,9 @@ void SplitWindow::ImplDrawFadeIn( sal_Bool bInPaint ) Image aImage; ImplGetFadeInRect( aTempRect ); -sal_Bool bLeft; -if ( meAlign == WINDOWALIGN_TOP ) -bLeft = sal_False; -else if ( meAlign == WINDOWALIGN_BOTTOM ) -bLeft = sal_True; -else if ( meAlign == WINDOWALIGN_LEFT ) -bLeft = sal_False; -else if ( meAlign == WINDOWALIGN_RIGHT ) -bLeft = sal_True; -else -bLeft = sal_True; +sal_Bool bLeft = sal_True; +if ( ( meAlign == WINDOWALIGN_TOP ) || ( meAlign == WINDOWALIGN_LEFT ) ) +bLeft = sal_False; if ( !bInPaint ) Erase( aTempRect ); ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
