Cppcheck reports
Hello, Since I build master sources and use cppcheck (C/C++ static analyzer) quite regularly, I thought someone could be interested by cppcheck reports. So, I updated today my master sources and launched cppcheck (git updated today) on the sources (except "sal" because of a freeze). Here the result: err.xml.bz2 <http://nabble.documentfoundation.org/file/n4028560/err.xml.bz2> Also, I uploaded detailed reports, I meant all files concerned by cppcheck reports + index page with links on source files "htmlized" You'll find it there: http://serval2412.free.fr/cppcheck_reports.tar.bz2 Just uncompress and browse Since it'll become quickly obsolete, I think I'll let this file just some days on ftp. Hope it helps. Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-tp4028560.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: Cppcheck reports
Just for information (I don't remember having done it already), I put cppcheck html reports here: http://dev-builds.libreoffice.org/cppcheck_reports/master/ I'm trying to update them from time to time (about once every 2 weeks or once per month when quite busy). Now I could update them more frequently if necessary. Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-tp4028560p4094974.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: Cppcheck reports
julien2412 wrote > Also, I uploaded detailed reports, I meant all files concerned by cppcheck > reports + index page with links on source files "htmlized" > You'll find it there: > http://serval2412.free.fr/cppcheck_reports.tar.bz2 > Just uncompress and browse Hi! Could this be placed at http://dev-builds.libreoffice.org/ joining clang_reports, lcov_reports and regenerated once in a while? Best regards. -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-tp4028560p4028571.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: Cppcheck reports
Hi bfo, I don't have access to this ftp but if someone wants to do it, no problem for me :-) About regenerating once a while, since it takes some time to retrieve all these reports, I can propose about once by week. (each time with master sources + cppcheck git updated of course :-)) Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-tp4028560p4028613.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: Cppcheck reports
Just for information, I updated the cppcheck reports (LO sources updated today + cppcheck updated today). I also created a first version to explain how to generate all this here: https://wiki.documentfoundation.org/Development/Cppcheck Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-tp4028560p4029780.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
Advice needed about some cppcheck reports
Hello,
Here are some cases found by cppcheck and I don't know what to do for them :
[sw/source/ui/docvw/SidebarWin.cxx:796] ->
[sw/source/ui/docvw/SidebarWin.cxx:794]: (style) Found duplicate branches
for if and else.
791 const SwViewOption* pVOpt =
mrView.GetWrtShellPtr()->GetViewOptions();
792 sal_uLong nCntrl = Engine()->GetControlWord();
793 // turn off
794 if (!pVOpt->IsOnlineSpell())
795 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
796 else
797 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
798 Engine()->SetControlWord(nCntrl);
799
800 //turn back on
801 if (pVOpt->IsOnlineSpell())
802 nCntrl |= EE_CNTRL_ONLINESPELLING;
803 else
804 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
805 Engine()->SetControlWord(nCntrl);
=> Just remove the if because we want to turn off in both cases ?
[sw/source/ui/shells/langhelper.cxx:214] ->
[sw/source/ui/shells/langhelper.cxx:212]: (style) Found duplicate branches
for if and else.
209 const SwViewOption* pVOpt =
rView.GetWrtShellPtr()->GetViewOptions();
210 sal_uLong nCntrl =
pEditEngine->GetControlWord();
211 // turn off
212 if (!pVOpt->IsOnlineSpell())
213 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
214 else
215 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
216 pEditEngine->SetControlWord(nCntrl);
217
218 //turn back on
219 if (pVOpt->IsOnlineSpell())
220 nCntrl |= EE_CNTRL_ONLINESPELLING;
221 else
222 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
223 pEditEngine->SetControlWord(nCntrl);
=> Idem former case ?
[connectivity/source/drivers/mozab/MDriver.cxx:240] ->
[connectivity/source/drivers/mozab/MDriver.cxx:238]: (style) Found duplicate
branches for if and else.
238 else if(url ==
::rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("sdbc:address:")) )
239 return Unknown; // TODO check
240 else
241 return Unknown;
In 2010-11-19 was in the form "else if(url ==
::rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("sdbc:address:")) )" and before,
like this since 2004-08-02
[sw/source/core/unocore/unomap.cxx:965] ->
[sw/source/core/unocore/unomap.cxx:965]: (style) Same expression on both
sides of '|'.
[sw/source/core/unocore/unomap.cxx:968] ->
[sw/source/core/unocore/unomap.cxx:968]: (style) Same expression on both
sides of '|'.
[sw/source/core/unocore/unomap.cxx:969] ->
[sw/source/core/unocore/unomap.cxx:969]: (style) Same expression on both
sides of '|'.
965 { SW_PROP_NMID(UNO_NAME_BACK_COLOR),
FN_UNO_TABLE_CELL_BACKGROUND, CPPU_E2T(CPPUTYPE_INT32),
PropertyAttribute::MAYBEVOID|PropertyAttribute::MAYBEVOID ,MID_BACK_COLOR
},
966 { SW_PROP_NMID(UNO_NAME_BACK_GRAPHIC_URL),
RES_BACKGROUND, CPPU_E2T(CPPUTYPE_OUSTRING),
PropertyAttribute::MAYBEVOID ,MID_GRAPHIC_URL},
967 { SW_PROP_NMID(UNO_NAME_BACK_GRAPHIC_FILTER),
RES_BACKGROUND, CPPU_E2T(CPPUTYPE_OUSTRING),
PropertyAttribute::MAYBEVOID ,MID_GRAPHIC_FILTER},
968 { SW_PROP_NMID(UNO_NAME_BACK_GRAPHIC_LOCATION),
FN_UNO_TABLE_CELL_BACKGROUND, CPPU_E2T(CPPUTYPE_GRAPHICLOC),
PropertyAttribute::MAYBEVOID|PropertyAttribute::MAYBEVOID
,MID_GRAPHIC_POSITION},
969 { SW_PROP_NMID(UNO_NAME_BACK_TRANSPARENT),
FN_UNO_TABLE_CELL_BACKGROUND,CPPU_E2T(CPPUTYPE_BOOLEAN),
PropertyAttribute::MAYBEVOID|PropertyAttribute::MAYBEVOID
,MID_GRAPHIC_TRANSPARENT },
=> just remove extra PropertyAttribute::MAYBEVOID ?
[sal/osl/unx/file.cxx:1261] -> [sal/osl/unx/file.cxx:1261]: (style) Same
expression on both sides of '-'.
1257 if (nSize > 0)
1258 {
1259 c^= pData[0];
1260 pData += nSize;
1261 nSize -= nSize;
1262 }
Just put nSize to 0 ?
Same thing here :
[sal/osl/w32/file.cxx:880] -> [sal/osl/w32/file.cxx:880]: (style) Same
expression on both sides of '-'.
876 if (nSize > 0)
877 {
878 c ^= pData[0];
879 pData += nSize;
880 nSize -= nSize;
881 }
And here :
[filter/source/graphicfilter/icgm/cgm.cxx:269] ->
[filter/source/graphicfilter/icgm/cgm.cxx:269]: (style) Same expression on
both sides of '-'.
267 if ( pLong[ nSwitch ] < 0 )
268 {
269 nRetValue -= nRetValue;
270 }
271
Cppcheck reports "Uninitialized variable bIsRow" in formulaparser.cxx
Hello,
Cppcheck reports this :
[source/filter/oox/formulaparser.cxx:2611]: (error) Uninitialized variable:
bIsRow
Here are the lines :
2608 bool BiffFormulaParserImpl::readNlrSAddrAddData( BiffNlr& orNlr,
BiffInputStream& rStrm, bool bRow )
2609 {
2610 bool bIsRow;
2611 return readNlrSRangeAddData( orNlr, bIsRow, rStrm ) && (bIsRow
== bRow);
2612 }
Now the function called is just the lines after :
bool BiffFormulaParserImpl::readNlrSRangeAddData( BiffNlr& orNlr, bool&
orbIsRow, BiffInputStream& rStrm )
Ok, so orbIsRow must be initialized on this function. The pb is it isn't
always the case.
Moreover "readNlrSRangeAddData" is called by 2 functions :
- BiffFormulaParserImpl::importNlrSRangeToken
- BiffFormulaParserImpl::readNlrSAddrAddData
Now the question, should the boolean variables be initialized by the 2
calling functions, if yes at which value ?
Or should there be a default value (which one ?) on readNlrSRangeAddData ?
Julien.
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-Uninitialized-variable-bIsRow-in-formulaparser-cxx-tp3836234p3836234.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: Advice needed about some cppcheck reports
On 28/05/12 18:03, julien2412 wrote:
> Hello,
>
> Here are some cases found by cppcheck and I don't know what to do for them :
> [sw/source/ui/docvw/SidebarWin.cxx:796] ->
> [sw/source/ui/docvw/SidebarWin.cxx:794]: (style) Found duplicate branches
> for if and else.
>
> 791 const SwViewOption* pVOpt =
> mrView.GetWrtShellPtr()->GetViewOptions();
> 792 sal_uLong nCntrl = Engine()->GetControlWord();
> 793 // turn off
> 794 if (!pVOpt->IsOnlineSpell())
> 795 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
> 796 else
> 797 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
> 798 Engine()->SetControlWord(nCntrl);
> 799
> 800 //turn back on
> 801 if (pVOpt->IsOnlineSpell())
> 802 nCntrl |= EE_CNTRL_ONLINESPELLING;
> 803 else
> 804 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
> 805 Engine()->SetControlWord(nCntrl);
> => Just remove the if because we want to turn off in both cases ?
yes, clearly the intent here is to turn it off always and then perhaps
back on.
> [sw/source/ui/shells/langhelper.cxx:214] ->
> [sw/source/ui/shells/langhelper.cxx:212]: (style) Found duplicate branches
> for if and else.
> 209 const SwViewOption* pVOpt =
> rView.GetWrtShellPtr()->GetViewOptions();
> 210 sal_uLong nCntrl =
> pEditEngine->GetControlWord();
> 211 // turn off
> 212 if (!pVOpt->IsOnlineSpell())
> 213 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
> 214 else
> 215 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
> 216 pEditEngine->SetControlWord(nCntrl);
> 217
> 218 //turn back on
> 219 if (pVOpt->IsOnlineSpell())
> 220 nCntrl |= EE_CNTRL_ONLINESPELLING;
> 221 else
> 222 nCntrl &= ~EE_CNTRL_ONLINESPELLING;
> 223 pEditEngine->SetControlWord(nCntrl);
> => Idem former case ?
yes
> [connectivity/source/drivers/mozab/MDriver.cxx:240] ->
> [connectivity/source/drivers/mozab/MDriver.cxx:238]: (style) Found duplicate
> branches for if and else.
> 238 else if(url ==
> ::rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("sdbc:address:")) )
> 239 return Unknown; // TODO check
> 240 else
> 241 return Unknown;
> In 2010-11-19 was in the form "else if(url ==
> ::rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("sdbc:address:")) )" and before,
> like this since 2004-08-02
no idea, presumably TODO indicates there should be special handling for
the sdbc:address case?
> [sw/source/core/unocore/unomap.cxx:965] ->
> [sw/source/core/unocore/unomap.cxx:965]: (style) Same expression on both
> sides of '|'.
> [sw/source/core/unocore/unomap.cxx:968] ->
> [sw/source/core/unocore/unomap.cxx:968]: (style) Same expression on both
> sides of '|'.
> [sw/source/core/unocore/unomap.cxx:969] ->
> [sw/source/core/unocore/unomap.cxx:969]: (style) Same expression on both
> sides of '|'.
> 965 { SW_PROP_NMID(UNO_NAME_BACK_COLOR),
> FN_UNO_TABLE_CELL_BACKGROUND, CPPU_E2T(CPPUTYPE_INT32),
> PropertyAttribute::MAYBEVOID|PropertyAttribute::MAYBEVOID ,MID_BACK_COLOR
>
> },
> 966 { SW_PROP_NMID(UNO_NAME_BACK_GRAPHIC_URL),
> RES_BACKGROUND, CPPU_E2T(CPPUTYPE_OUSTRING),
> PropertyAttribute::MAYBEVOID ,MID_GRAPHIC_URL},
> 967 { SW_PROP_NMID(UNO_NAME_BACK_GRAPHIC_FILTER),
> RES_BACKGROUND, CPPU_E2T(CPPUTYPE_OUSTRING),
> PropertyAttribute::MAYBEVOID ,MID_GRAPHIC_FILTER},
> 968 { SW_PROP_NMID(UNO_NAME_BACK_GRAPHIC_LOCATION),
> FN_UNO_TABLE_CELL_BACKGROUND, CPPU_E2T(CPPUTYPE_GRAPHICLOC),
> PropertyAttribute::MAYBEVOID|PropertyAttribute::MAYBEVOID
> ,MID_GRAPHIC_POSITION},
> 969 { SW_PROP_NMID(UNO_NAME_BACK_TRANSPARENT),
> FN_UNO_TABLE_CELL_BACKGROUND,CPPU_E2T(CPPUTYPE_BOOLEAN),
> PropertyAttribute::MAYBEVOID|PropertyAttribute::MAYBEVOID
> ,MID_GRAPHIC_TRANSPARENT },
> => just remove extra PropertyAttribute::MAYBEVOID ?
it looks to me like none of the other PropertyAttribute values make
sense here, so just remove the duplicate.
> [sal/osl/unx/file.cxx:1261] -> [sal/osl/unx/file.cxx:1261]: (style) Same
> expression on both sides of '-'.
>1257 if (nSize > 0)
>1258 {
>1259 c^= pData[0];
>1260 pData += nSize;
>1261 nSize -= nSize;
>1262 }
> Just put nSize to 0 ?
makes sense
> Same thing here :
> [sal/osl/w32/file.cxx:880] -> [sal/osl/w32/file.cxx:880]: (style) Same
> expression on both sides of '-'.
> 876 if (nSize > 0)
> 877 {
> 878 c ^= pData[0];
> 879 pData += nSize;
> 880
Re: Advice needed about some cppcheck reports
On Tuesday 29 of May 2012, Michael Stahl wrote:
> On 28/05/12 18:03, julien2412 wrote:
> > Same thing here :
> > [sal/osl/w32/file.cxx:880] -> [sal/osl/w32/file.cxx:880]: (style) Same
> > expression on both sides of '-'.
> > 876 if (nSize > 0)
> > 877 {
> > 878 c ^= pData[0];
> > 879 pData += nSize;
> > 880 nSize -= nSize;
> > 881 }
> >
> > And here :
> > [filter/source/graphicfilter/icgm/cgm.cxx:269] ->
> > [filter/source/graphicfilter/icgm/cgm.cxx:269]: (style) Same expression
> > on both sides of '-'.
> > 267 if ( pLong[ nSwitch ] < 0 )
> > 268 {
> > 269 nRetValue -= nRetValue;
> > 270 }
> > 271 nRetValue /= 65536;
>
> also makes sense
This is not the same, it looks more to me like it should be 'nRetValue
= -nRetValue;' to negate the value, especially given the abs() above.
--
Lubos Lunak
l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Advice needed about some cppcheck reports
On 29/05/12 16:04, Lubos Lunak wrote:
> On Tuesday 29 of May 2012, Michael Stahl wrote:
>> On 28/05/12 18:03, julien2412 wrote:
>>> Same thing here :
>>> [sal/osl/w32/file.cxx:880] -> [sal/osl/w32/file.cxx:880]: (style) Same
>>> expression on both sides of '-'.
>>> 876 if (nSize > 0)
>>> 877 {
>>> 878 c ^= pData[0];
>>> 879 pData += nSize;
>>> 880 nSize -= nSize;
>>> 881 }
>>>
>>> And here :
>>> [filter/source/graphicfilter/icgm/cgm.cxx:269] ->
>>> [filter/source/graphicfilter/icgm/cgm.cxx:269]: (style) Same expression
>>> on both sides of '-'.
>>> 267 if ( pLong[ nSwitch ] < 0 )
>>> 268 {
>>> 269 nRetValue -= nRetValue;
>>> 270 }
>>> 271 nRetValue /= 65536;
>>
>> also makes sense
>
> This is not the same, it looks more to me like it should be 'nRetValue
> = -nRetValue;' to negate the value, especially given the abs() above.
d'oh, thanks for the correction, stupid me had overlooked that there
were 3 of these, i've only seen the first 2 but then added this comment
below the third one which i hadn't actually looked at :-/
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Advice needed about some cppcheck reports
On 05/28/2012 06:03 PM, julien2412 wrote:
[sal/osl/unx/file.cxx:1261] -> [sal/osl/unx/file.cxx:1261]: (style) Same
expression on both sides of '-'.
1257 if (nSize> 0)
1258 {
1259 c^= pData[0];
1260 pData += nSize;
1261 nSize -= nSize;
1262 }
Just put nSize to 0 ?
Same thing here :
[sal/osl/w32/file.cxx:880] -> [sal/osl/w32/file.cxx:880]: (style) Same
expression on both sides of '-'.
876 if (nSize> 0)
877 {
878 c ^= pData[0];
879 pData += nSize;
880 nSize -= nSize;
881 }
In both of the above cases, the updates of both pData and nSize can be
removed completely from these "overspill" if-blocks, as the variables
are not used any more afterwards.
And here :
[filter/source/graphicfilter/icgm/cgm.cxx:269] ->
[filter/source/graphicfilter/icgm/cgm.cxx:269]: (style) Same expression on
both sides of '-'.
267 if ( pLong[ nSwitch ]< 0 )
268 {
269 nRetValue -= nRetValue;
270 }
271 nRetValue /= 65536;
I'd second Lubos here, that from looking at just the code in
CGM::ImplGetFloat (but not understanding any of the context), "nRetValue
= -nRetValue" is most likely what is meant instead.
Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "Uninitialized variable bIsRow" in formulaparser.cxx
On Sun, 2012-03-18 at 02:41 -0700, julien2412 wrote: ... > Or should there be a default value (which one ?) on readNlrSRangeAddData ? Let just do that. pushed now as 735a2cba30c55b5aff4883389f65255fb2d07458 I suspect these things might be at the bottom of an unused code trail myself. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[PARTLY PUSHED] Re: Advice needed about some cppcheck reports
Thank you Michael and Lubos for your feedback. I commited and pushed on master some elements (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=40106014f7b5c414faf087472a5ae350e683db53) I put aside for the moment : connectivity/source/drivers/mozab/MDriver.cxx:240 (we don't know yet what to do) sal/osl/w32/file.cxx:880 (since I'm not sure having well understood the "abs" and "nRet" thing :-() filter/source/graphicfilter/icgm/cgm.cxx:269 (no response about this for the moment) Julien. -- View this message in context: http://nabble.documentfoundation.org/Advice-needed-about-some-cppcheck-reports-tp3986408p3986586.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
Cppcheck reports "Division by zero" in new.cxx (sfx2 module)
Hello, Cppcheck reported this: sfx2/source/doc/new.cxx 81 zerodiv error Division by zero. Indeed we have this: 74 #define FRAME 4 75 long nWidth = pWindow->GetOutputSize().Width() - 2*FRAME; 76 long nHeight = pWindow->GetOutputSize().Height() - 2*FRAME; 77 if( nWidth < 0 ) nWidth = 0; 78 if( nHeight < 0 ) nHeight = 0; 79 80 double dRatio=((double)aTmpSize.Width())/aTmpSize.Height(); 81 double dRatioPreV=((double) nWidth ) / nHeight; (see http://opengrok.libreoffice.org/xref/core/sfx2/source/doc/new.cxx#74) Should line 78 be replaced by: if( nHeight < 0 ) nHeight = 1; or should we do something completely different? Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Division-by-zero-in-new-cxx-sfx2-module-tp4102613.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
Cppcheck reports 'else if' condition matches previous condition (svx)
Hello,
Cppcheck reported this
svx/source/unodraw/unomod.cxx
492 multiCondition style Expression is always false because 'else if'
condition matches previous condition at line 460.
Remark: It's a new kind of cppcheck detection and there are quite a lot of
false positives (at least for LO) for the moment.
Here's the code:
460 else if( aTypeName.startsWith( "TableShape" ) )
461 {
462 nType = OBJ_OLE2;
463 }
...
492 else if( aTypeName.startsWith( "TableShape" ) )
493 {
494 nType = OBJ_TABLE;
495 }
see
http://opengrok.libreoffice.org/xref/core/svx/source/unodraw/unomod.cxx#460
Which one of this block is ok?
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-else-if-condition-matches-previous-condition-svx-tp4104268.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
Cppcheck reports 'else if' condition matches previous condition (vcl)
Hello,
Cppcheck reported this:
vcl/source/gdi/impimage.cxx
300 multiCondition style Expression is always false because 'else if'
condition matches previous condition at line 298.
Indeed we have:
296 else
297 {
298 if( aTmpBmpEx.IsAlpha() )
299 aTmpBmpEx = BitmapEx( aTmpBmp,
aTmpBmpEx.GetAlpha() );
300 else if( aTmpBmpEx.IsAlpha() )
301 aTmpBmpEx = BitmapEx( aTmpBmp,
aTmpBmpEx.GetMask() );
302 }
see
http://opengrok.libreoffice.org/xref/core/vcl/source/gdi/impimage.cxx#298
Should the else if be:
else if( aTmpBmpEx.IsTransparent())
?
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-else-if-condition-matches-previous-condition-vcl-tp4104270.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
Cppcheck reports "Same expression on both sides of '=='" on dpitemdata.cxx
Hello,
Just to make notice that Cppcheck reports "Same expression on both sides of
'=='" on sc/source/core/data/dpitemdata.cxx, line 217. Here are the lines :
199 bool ScDPItemData::IsCaseInsEqual(const ScDPItemData& r) const
200 {
201 if (meType != r.meType)
202 return false;
203
204 switch (meType)
205 {
206 case Value:
207 case RangeStart:
208 return rtl::math::approxEqual(mfValue, r.mfValue);
209 case GroupValue:
210 return maGroupValue.mnGroupType ==
r.maGroupValue.mnGroupType &&
211 maGroupValue.mnValue == r.maGroupValue.mnValue;
212 default:
213 ;
214 }
215
216 if (mbStringInterned && r.mbStringInterned)
217 return mpString == mpString; < HERE
218
219 return ScGlobal::GetpTransliteration()->isEqual(GetString(),
r.GetString());
220 }
It's the commit f81d15c3bab32938b5b475e16ae2a746a7a32ea9 (17/03/2012).
Julien.
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-Same-expression-on-both-sides-of-on-dpitemdata-cxx-tp3836145p3836145.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
Cppcheck reports "Logical conjunction always evaluates to false" in text_gfx.cxx
Hello, Cppcheck reports this : [generic/print/text_gfx.cxx:105]: (warning) Logical conjunction always evaluates to false: nChar < 65380 && nChar >= 65387 Here are the lines : 103 if( ( nChar >= 0x3008 && nChar < 0x3019 && nChar != 0x3012 ) || 104 nChar == 0xff3b || nChar == 0xff3d || 105 (nChar >= 0xff6b && nChar < 0xff64 ) || 106 nChar == 0xffe3 107 ) 108 nAngle = 0; Shoud the line 105 just be replaced by : (nChar >= 0xff64 && nChar < 0xff6b ) || Or is it less obvious/more tricky than that ? If ok, I can commit and push it on master of course. Julien. -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Logical-conjunction-always-evaluates-to-false-in-text-gfx-cxx-tp3836511p3836511.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
Cppcheck reports "Same expression on both sides" in writerfilter module
Hello,
On master repo, cppcheck reported this :
[writerfilter/source/filter/ImportFilter.cxx:227] ->
[writerfilter/source/filter/ImportFilter.cxx:227]: (style) Same expression
on both sides of '||'
Here are the lines :
225 sal_Bool WriterFilter_supportsService( const OUString& ServiceName )
throw (uno::RuntimeException)
226 {
227return (ServiceName == SERVICE_NAME1 || ServiceName ==
SERVICE_NAME1 );
228 }
I would just replace the second "SERVICE_NAME1" by SERVICE_NAME2 and here
why :
I took a look at git history and found this :
1) 22/02/2007, with this commit f6badd4232da41601fa09b86d87a925c48f97e75,
SERVICE_NAME2 has been removed.
We can notice this :
-#define SERVICE_NAME2 "com.sun.star.document.ExtendedTypeDetection"
2) 25/08/2009, with this commit 2780d8bec4d34ff177a0d6428b1f709c17f5c9c7,
SERVICE_NAME2 reappeared but there has been an error in
"WriterFilter_supportsService" function cause SERVICE_NAME1 appears twice in
it.
We can notice this :
+#define SERVICE_NAME2 "com.sun.star.document.ExportFilter"
So because of the 2 things noticed, I think there should be both and so just
replace "1" by "2" and have "SERVICE_NAME2" as second operand.
Perhaps I'm wrong or missed something, any idea ?
Julien.
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-Same-expression-on-both-sides-in-writerfilter-module-tp3893599p3893599.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: Cppcheck reports "Division by zero" in new.cxx (sfx2 module)
On Sat, 2014-03-22 at 06:50 -0700, julien2412 wrote: > or should we do something completely different? I'd just return early if the width or height is <= 0. Though I think I'd move the SetLineColor/SetFillColor and "DrawRect" to the top of the method so 4 pixel high/wide windows still get blanked out. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Cppcheck reports Using 'memset' on class that contains ... (sw module)
Hello, Cppcheck reported this: sw/source/filter/ww8/ww8scan.cxx 5158memsetClass error Using 'memset' on class that contains a virtual method. 5158memsetClass error Using 'memset' on class that contains a reference. 5158memsetClass error Using 'memset' on class that contains a 'std::map'. See http://opengrok.libreoffice.org/xref/core/sw/source/filter/ww8/ww8scan.cxx#5155 False positive or real problem? Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Using-memset-on-class-that-contains-sw-module-tp4103435.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
Cppcheck reports Same expression on both sides of '&&' (sc module)
Hello, Cppcheck reported this: sc/source/core/opencl/op_statistical.cxx 3221duplicateExpression style Same expression on both sides of '&&'. 3233duplicateExpression style Same expression on both sides of '&&'. 3245duplicateExpression style Same expression on both sides of '&&'. 3305duplicateExpression style Same expression on both sides of '&&'. 3317duplicateExpression style Same expression on both sides of '&&'. eg: 3220 else if ((pDVR->IsStartFixed() && !pDVR->IsEndFixed()) 3221 &&(pDVR->IsStartFixed() && !pDVR->IsEndFixed())) (see http://opengrok.libreoffice.org/xref/core/sc/source/core/opencl/op_statistical.cxx#3220) Should the second line use pDVR1 (like some other parts), should the second line be removed each time, or something else should be done? Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Same-expression-on-both-sides-of-sc-module-tp4103436.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: Cppcheck reports 'else if' condition matches previous condition (svx)
On Sat, 2014-04-05 at 15:51 -0700, julien2412 wrote:
> Hello,
>
> Cppcheck reported this
> svx/source/unodraw/unomod.cxx
> 492 multiCondition style Expression is always false because 'else if'
> condition matches previous condition at line 460.
>
> Remark: It's a new kind of cppcheck detection and there are quite a lot of
> false positives (at least for LO) for the moment.
>
> Here's the code:
> 460 else if( aTypeName.startsWith( "TableShape" ) )
> 461 {
> 462 nType = OBJ_OLE2;
> 463 }
> ...
> 492 else if( aTypeName.startsWith( "TableShape" ) )
> 493 {
> 494 nType = OBJ_TABLE;
> 495 }
>
> see
> http://opengrok.libreoffice.org/xref/core/svx/source/unodraw/unomod.cxx#460
>
> Which one of this block is ok?
I rather think the second one is the correct one, seeing as
svx/source/unodraw/unopage.cxx maps TableShape to OBJ_TABLE as well.
At one point tables in impress/draw were embedded calc spreadsheets, and
now they are "real" SdrObject things to that would also make sense.
On the other hand that will change the current situation and I have no
idea what makes the code enter that method so definitely a make check
case at least :-)
C.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports 'else if' condition matches previous condition (svx)
On 08/04/2014 13:10, Caolán McNamara wrote:
On Sat, 2014-04-05 at 15:51 -0700, julien2412 wrote:
Hello,
Cppcheck reported this
svx/source/unodraw/unomod.cxx
492 multiCondition style Expression is always false because 'else if'
condition matches previous condition at line 460.
Remark: It's a new kind of cppcheck detection and there are quite a lot of
false positives (at least for LO) for the moment.
Here's the code:
460 else if( aTypeName.startsWith( "TableShape" ) )
461 {
462 nType = OBJ_OLE2;
463 }
...
492 else if( aTypeName.startsWith( "TableShape" ) )
493 {
494 nType = OBJ_TABLE;
495 }
see
http://opengrok.libreoffice.org/xref/core/svx/source/unodraw/unomod.cxx#460
Which one of this block is ok?
I rather think the second one is the correct one, seeing as
svx/source/unodraw/unopage.cxx maps TableShape to OBJ_TABLE as well.
At one point tables in impress/draw were embedded calc spreadsheets, and
now they are "real" SdrObject things to that would also make sense.
On the other hand that will change the current situation and I have no
idea what makes the code enter that method so definitely a make check
case at least :-)
I've just pushed the fix on master. "make check" on top level was ok.
Searching in git history during "make check", I found this commit from 2008:
http://cgit.freedesktop.org/libreoffice/core/commit/?id=5d20a47c3d50d0a88543b2355ec7340fc7455984
which references "OBJ_TABLE"
whereas OBJ_OLE2 was in 2001 (see
http://cgit.freedesktop.org/libreoffice/core/commit/?id=e24bc241b69da6789351d9bf82eab5119f09c16c)
I submitted a gerrit review 4.2:
https://gerrit.libreoffice.org/8897
Julien
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports 'else if' condition matches previous condition (svx)
On Tue, 2014-04-08 at 20:58 +0200, Julien Nabet wrote: > I've just pushed the fix on master. "make check" on top level was ok. > Searching in git history during "make check", I found this commit from 2008: > http://cgit.freedesktop.org/libreoffice/core/commit/?id=5d20a47c3d50d0a88543b2355ec7340fc7455984 > which references "OBJ_TABLE" > whereas OBJ_OLE2 was in 2001 (see > http://cgit.freedesktop.org/libreoffice/core/commit/?id=e24bc241b69da6789351d9bf82eab5119f09c16c) > > > > I submitted a gerrit review 4.2: > https://gerrit.libreoffice.org/8897 I'm rather reluctant to change this in 4.2 without a specific known problem that it fixes. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports 'else if' condition matches previous condition (vcl)
On Sat, 2014-04-05 at 16:06 -0700, julien2412 wrote:
> Indeed we have:
> 296 else
> 297 {
> 298 if( aTmpBmpEx.IsAlpha() )
> 299 aTmpBmpEx = BitmapEx( aTmpBmp,
> aTmpBmpEx.GetAlpha() );
> 300 else if( aTmpBmpEx.IsAlpha() )
> 301 aTmpBmpEx = BitmapEx( aTmpBmp,
> aTmpBmpEx.GetMask() );
> 302 }
> see
> http://opengrok.libreoffice.org/xref/core/vcl/source/gdi/impimage.cxx#298
>
> Should the else if be:
> else if( aTmpBmpEx.IsTransparent())
"grep -r IsAlpha -C 5 vcl|grep else" definitely strongly suggests that
was the intent. I reckon its worth making that change and see if
anything falls over. Not the kind of thing I'd backport to 4-2 unless we
get compelling evidence to do that.
C.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Question cppcheck reports on svx/source/svdraw/svdoedge.cxx (Bezier curve)
Hello, Cppcheck detected this into svx/source/svdraw/svdoedge.cxx [svdoedge.cxx:1450]: (style) Variable 'pPt1' is assigned a value that is never used. [svdoedge.cxx:1453]: (style) Variable 'pPt4' is assigned a value that is never used Here are the lines: 1450 pPt1=&aXP1[0]; 1451 pPt2=&aXP1[1]; 1452 pPt3=&aXP1[nPntAnz-2]; 1453 pPt4=&aXP1[nPntAnz-1]; 1454 pPt2->X()-=dx1/3; 1455 pPt2->Y()-=dy1/3; 1456 pPt3->X()-=dx2/3; 1457 pPt3->Y()-=dy2/3; and Opengrok: http://opengrok.libreoffice.org/xref/core/svx/source/svdraw/svdoedge.cxx#1436 Either pPt1 and pPt4 assignations may really be removed or a change must be done here. Any idea? Julien -- View this message in context: http://nabble.documentfoundation.org/Question-cppcheck-reports-on-svx-source-svdraw-svdoedge-cxx-Bezier-curve-tp4025235.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: Cppcheck reports "Same expression on both sides of '=='" on dpitemdata.cxx
Hi On 18.03.2012 12:41, julien2412 wrote: Just to make notice that Cppcheck reports "Same expression on both sides of '=='" on sc/source/core/data/dpitemdata.cxx, line 217. Here are the lines : ... 216 if (mbStringInterned && r.mbStringInterned) 217 return mpString == mpString;< HERE Wow! There definitely must be "return mpString == r.mpString". Probably Kohei would like to redo his performance tests... Do you prefer to push fixes yourself? :) Thanks!!! Ivan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Cppcheck reports "Logical disjunction always evaluates to true" in sdext module
Hello, Cppcheck reports this in sdext module : [source/minimizer/optimizerdialog.cxx:276]: (warning) Logical disjunction always evaluates to true: nNewStep >= 0 || nNewStep <= 4 Here is the line : if ( ( nNewStep != mnCurrentStep ) && ( ( nNewStep <= MAX_STEP ) || ( nNewStep >= 0 ) ) ) Shouldn't it be : if ( ( nNewStep != mnCurrentStep ) && ( nNewStep <= MAX_STEP ) && ( nNewStep >= 0 ) ) ? Moreover, if I'm right, should we additionnally throw an exception (which one ?) if "nNewStep" is not between 0 and MAX_STEP If ok for just the condition change, I can commit and push on master (what about 3.5 ?) About the exception, I don't know what to put. Julien. -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Logical-disjunction-always-evaluates-to-true-in-sdext-module-tp3836919p3836919.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: Cppcheck reports "Logical conjunction always evaluates to false" in text_gfx.cxx
On 03/18/2012 02:39 PM, julien2412 wrote: Cppcheck reports this : [generic/print/text_gfx.cxx:105]: (warning) Logical conjunction always Would be helpful if you gave the complete path, vcl/generic/print/text_gfx.cxx. evaluates to false: nChar< 65380&& nChar>= 65387 Here are the lines : 103 if( ( nChar>= 0x3008&& nChar< 0x3019&& nChar != 0x3012 ) || 104 nChar == 0xff3b || nChar == 0xff3d || 105 (nChar>= 0xff6b&& nChar< 0xff64 ) || 106 nChar == 0xffe3 107 ) 108 nAngle = 0; Shoud the line 105 just be replaced by : (nChar>= 0xff64&& nChar< 0xff6b ) || Or is it less obvious/more tricky than that ? From those Unicode characters (mostly CJK brackets), it looks like that should probably be nChar >= 0xff62 && nChar < 0xff64 i.e., matching U+FF62 HALFWIDTH LEFT CORNER BRACKET and U+FF63 HALFWIDTH RIGHT CORNER BRACKET. Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "Logical conjunction always evaluates to false" in text_gfx.cxx
Here is a patch : http://nabble.documentfoundation.org/file/n3840411/text_gfx_patch.txt text_gfx_patch.txt remarks : - I replaced this nChar >= 0xff62 && nChar < 0xff64 by this for readability nChar == 0xff62 || nChar == 0xff63 - I added in the outer "if", "nChar == 0xffe3" because if not, "nChar == 0xffe3" in the inner "if" is useless. Is this patch ok ? If yes, I can commit and push it on master. Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Logical-conjunction-always-evaluates-to-false-in-text-gfx-cxx-tp3836511p3840411.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: Cppcheck reports "Logical conjunction always evaluates to false" in text_gfx.cxx
On 03/19/2012 10:25 PM, julien2412 wrote: remarks : - I replaced this nChar>= 0xff62&& nChar< 0xff64 by this for readability nChar == 0xff62 || nChar == 0xff63 - I added in the outer "if", "nChar == 0xffe3" because if not, "nChar == 0xffe3" in the inner "if" is useless. Is this patch ok ? If yes, I can commit and push it on master. I at least know just as little as anybody else whether this is right or not (the U+FFE3 FULLWIDTH MACRON still looks odd here, but who knows). Presumably the patch is fine, but to know for sure somebody would have to dig out what exactly the code is actually supposed to do. Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "Logical conjunction always evaluates to false" in text_gfx.cxx
Just for the update, is there anybody who could help about this ? Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Logical-conjunction-always-evaluates-to-false-in-text-gfx-cxx-tp3836511p3852917.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: Cppcheck reports "Logical conjunction always evaluates to false" in text_gfx.cxx
Hi Julien, On 24.03.2012 03:26, julien2412 wrote: Just for the update, is there anybody who could help about this ? I am not expert, too, but looking at vcl/source/gdi/sallayout.cxx:115 and http://www.unicode.org/charts/PDF/UFF00.pdf I'd say that the condition should be at least (nChar >= 0xff5b && nChar < 0xff64 ) ^ However, I do not know why this implementation (in text_gfx.cxx) differs from sallayout.cxx. Regards, Ivan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "Logical conjunction always evaluates to false" in text_gfx.cxx
On Mon, 2012-03-26 at 18:03 +0400, Ivan Timofeev wrote: > Hi Julien, > > On 24.03.2012 03:26, julien2412 wrote: > > Just for the update, is there anybody who could help about this ? > > I am not expert, too, but looking at vcl/source/gdi/sallayout.cxx:115 > and http://www.unicode.org/charts/PDF/UFF00.pdf I'd say that the > condition should be at least >(nChar >= 0xff5b && nChar < 0xff64 ) > ^ > However, I do not know why this implementation (in text_gfx.cxx) differs > from sallayout.cxx. Quite probably the one in text_gfx.cxx lived in a different toplevel "psprint" modules ages ago, and got moved into vcl when that module finally got merged into vcl. There was loads of sort-of-duplicated stuff between psprint and vcl. I say we just merge the two things together like so... http://cgit.freedesktop.org/libreoffice/core/commit/?id=7d6656addebcaec4f8873e4a611a970a224a7bf5 C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Cppcheck reports "Same expression on both sides of '|'" on basctl module
Hello,
On master, cppcheck reported this :
[basctl/source/basicide/bastypes.cxx:269] ->
[basctl/source/basicide/bastypes.cxx:269]: (style) Same expression on both
sides of '|'.
Here are the lines :
266 BasicDockingWindow::BasicDockingWindow( Window* pParent, const
ResId& rResId ) :
267 DockingWindow( pParent, rResId )
268 {
269 SetStyle( WB_BORDER | WB_3DLOOK | WB_DOCKABLE | WB_MOVEABLE |
270 WB_SIZEABLE | WB_ROLLABLE |
271 WB_DOCKABLE | WB_CLIPCHILDREN );
272 }
It's quite recent, 1 month ago with commit
0e8eb19a53338c83dab7fe19e2f23bcaecd52077.
I've got not idea, if we can just remove WB_DOCKABLE or if it should be
replaced by another constant.
Any advice ?
Julien.
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-Same-expression-on-both-sides-of-on-basctl-module-tp3893608p3893608.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: Cppcheck reports "Same expression on both sides" in writerfilter module
On Sunday 08 of April 2012, julien2412 wrote: > Hello, > > On master repo, cppcheck reported this : > [writerfilter/source/filter/ImportFilter.cxx:227] -> > [writerfilter/source/filter/ImportFilter.cxx:227]: (style) Same expression > on both sides of '||' ... > So because of the 2 things noticed, I think there should be both and so > just replace "1" by "2" and have "SERVICE_NAME2" as second operand. > > Perhaps I'm wrong or missed something, any idea ? No, your analysis looks correct to me, feel free to push the fix. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Question cppcheck reports on svx/source/svdraw/svdoedge.cxx (Bezier curve)
On Wed, 2012-12-19 at 15:00 -0800, julien2412 wrote: > Hello, > > Cppcheck detected this into svx/source/svdraw/svdoedge.cxx > [svdoedge.cxx:1450]: (style) Variable 'pPt1' is assigned a value that is > never used. > [svdoedge.cxx:1453]: (style) Variable 'pPt4' is assigned a value that is > never used That SdrEdgeObj::ImpCalcEdgeTrack is one of the most unpleasantly long and unreadable things I've seen in LibreOffice. > Either pPt1 and pPt4 assignations may really be removed or a change must be > done here. git blame && show on results indicate it's there since initial OOo-ification in 2000. It looks like a dead loss to me to do anything except remove the assignations that have no utility. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[SOLVED] Re: Cppcheck reports "Division by zero" in new.cxx (sfx2 module)
On 24/03/2014 14:38, Caolán McNamara wrote: On Sat, 2014-03-22 at 06:50 -0700, julien2412 wrote: or should we do something completely different? I'd just return early if the width or height is <= 0. Though I think I'd move the SetLineColor/SetFillColor and "DrawRect" to the top of the method so 4 pixel high/wide windows still get blanked out. Thank you Caolán for your feedback. I've pushed the patch here: http://cgit.freedesktop.org/libreoffice/core/commit/?id=59475ae16329edd32d286eb912c3f52d3395c818 Julien ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports Using 'memset' on class that contains ... (sw module)
On 03/29/2014 01:49 PM, julien2412 wrote: Cppcheck reported this: sw/source/filter/ww8/ww8scan.cxx 5158memsetClass error Using 'memset' on class that contains a virtual method. 5158memsetClass error Using 'memset' on class that contains a reference. 5158memsetClass error Using 'memset' on class that contains a 'std::map'. While the latter two would be real errors (though I don't see a good reason to warn about the first), I fail to find any reference or std::map members (or virtual function members, for that matter) in the definition of class WW8Fib in recent trunk sw/source/filter/ww8/ww8scan.hxx. (And it doesn't add to your confidence in Cppcheck if you see it use such bad language as "virtual method.") Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[SOLVED] Re: Cppcheck reports 'else if' condition matches previous condition (svx)
On 11/04/2014 13:20, Caolán McNamara wrote: On Tue, 2014-04-08 at 20:58 +0200, Julien Nabet wrote: ... I submitted a gerrit review 4.2: https://gerrit.libreoffice.org/8897 I'm rather reluctant to change this in 4.2 without a specific known problem that it fixes. Ok then, I abandonned it. Julien ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[SOLVED] Re: Cppcheck reports 'else if' condition matches previous condition (vcl)
On 11/04/2014 16:13, Caolán McNamara wrote:
On Sat, 2014-04-05 at 16:06 -0700, julien2412 wrote:
Indeed we have:
296 else
297 {
298 if( aTmpBmpEx.IsAlpha() )
299 aTmpBmpEx = BitmapEx( aTmpBmp,
aTmpBmpEx.GetAlpha() );
300 else if( aTmpBmpEx.IsAlpha() )
301 aTmpBmpEx = BitmapEx( aTmpBmp,
aTmpBmpEx.GetMask() );
302 }
see
http://opengrok.libreoffice.org/xref/core/vcl/source/gdi/impimage.cxx#298
Should the else if be:
else if( aTmpBmpEx.IsTransparent())
"grep -r IsAlpha -C 5 vcl|grep else" definitely strongly suggests that
was the intent. I reckon its worth making that change and see if
anything falls over. Not the kind of thing I'd backport to 4-2 unless we
get compelling evidence to do that.
Thank you Caolán, I pushed the change in master, see
http://cgit.freedesktop.org/libreoffice/core/commit/?id=6c6f78c6567db73aa85de4beb461db2cddac7b2a
Julien
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Cppcheck reports Resource leak in setup_native/source/win32/wintools/makecab/parseddf.c
Hello, cppcheck reported this: [setup_native/source/win32/wintools/makecab/parseddf.c:381]: (error) Resource leak: ddf it's in the function ParseDdf fclose function seems to miss. (see http://opengrok.libreoffice.org/xref/core/setup_native/source/win32/wintools/makecab/parseddf.c#356) I'm not able to build on Windows but I think that adding this line: fclose (ddf); before this line: return DDF_OK; should be ok. Someone to confirm? Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Resource-leak-in-setup-native-source-win32-wintools-makecab-parseddf-c-tp4026214.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
Cppcheck reports an 2 assignments which isn't used in drawinglayer module
Hello,
Cppcheck reported these:
[drawinglayer/source/primitive2d/sceneprimitive2d.cxx:196]: (style) Variable
'fViewSizeX' is assigned a value that is never used.
[drawinglayer/source/primitive2d/sceneprimitive2d.cxx:197]: (style) Variable
'fViewSizeY' is assigned a value that is never used.
193 if(fReducedVisualisationFactor != 1.0)
194 {
195 fReduceFactor *=
fReducedVisualisationFactor;
196 fViewSizeX *= fReducedVisualisationFactor;
197 fViewSizeY *= fReducedVisualisationFactor;
198 }
see
http://opengrok.libreoffice.org/xref/core/drawinglayer/source/primitive2d/sceneprimitive2d.cxx#193
Indeed, fViewSizeX and fViewSizeY. this time I don't think they should be
removed, I rather think something is lacking.
Any idea?
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-an-2-assignments-which-isn-t-used-in-drawinglayer-module-tp4026316.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
Cppcheck reports 'nMaxWidth' is assigned a value never used (svtools module)
Hello,
Cppcheck reports this:
svtools/source/dialogs/filedlg2.cxx
558 unreadVariable style Variable 'nMaxWidth' is assigned a value that is
never used.
544 if( nMaxWidth > aSize.Width() )
545 {
546 Size aDlgSize = GetPathDialog()->GetOutputSizePixel();
547 GetPathDialog()->SetOutputSizePixel( Size(
aDlgSize.Width()+nMaxWidth-aSize.Width(), aDlgSize.Height() ) );
548 aSize.Width() = nMaxWidth;
549
550 if( pOkBtn )
551 pOkBtn->SetSizePixel( aSize );
552 if( pCancelBtn )
553 pCancelBtn->SetSizePixel( aSize );
554 if( pLoadBtn )
555 pLoadBtn->SetSizePixel( aSize );
556 }
557 else
558 nMaxWidth = aSize.Width();
see
http://opengrok.libreoffice.org/xref/core/svtools/source/dialogs/filedlg2.cxx#544
Any idea what should be done with this?
Remark:
By trying to understand this, I wondered how come line 548 was ok, then I
noticed these lines in tools/inc/tools/gen.hxx:
longWidth() const { return nA; }
long&Width() const { return nA; }
I must recognize I don't understand the interest since there's a setWidth
function
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-nMaxWidth-is-assigned-a-value-never-used-svtools-module-tp4036771.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
[PUSHED] Re: Cppcheck reports "Same expression on both sides of '=='" on dpitemdata.cxx
On 18/03/2012 15:07, Ivan Timofeev wrote: Hi On 18.03.2012 12:41, julien2412 wrote: Just to make notice that Cppcheck reports "Same expression on both sides of '=='" on sc/source/core/data/dpitemdata.cxx, line 217. Here are the lines : ... 216 if (mbStringInterned && r.mbStringInterned) 217 return mpString == mpString;< HERE Wow! There definitely must be "return mpString == r.mpString". Probably Kohei would like to redo his performance tests... Do you prefer to push fixes yourself? :) Fix pushed and commited on master (2b24cfe22d5d29645d2d926251c29514887fe3a9) Thank you for the review ! :-) Julien. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "Same expression on both sides of '|'" on basctl module
On Sunday 08 of April 2012, julien2412 wrote:
> Hello,
>
> On master, cppcheck reported this :
> [basctl/source/basicide/bastypes.cxx:269] ->
> [basctl/source/basicide/bastypes.cxx:269]: (style) Same expression on both
> sides of '|'.
> Here are the lines :
> 266 BasicDockingWindow::BasicDockingWindow( Window* pParent, const
> ResId& rResId ) :
> 267 DockingWindow( pParent, rResId )
> 268 {
> 269 SetStyle( WB_BORDER | WB_3DLOOK | WB_DOCKABLE | WB_MOVEABLE |
> 270 WB_SIZEABLE | WB_ROLLABLE |
> 271 WB_DOCKABLE | WB_CLIPCHILDREN );
> 272 }
>
> It's quite recent, 1 month ago with commit
> 0e8eb19a53338c83dab7fe19e2f23bcaecd52077.
The problem actually has been there for over 10 years at least, the commit
you mention just copied it from those few lines above, where there is the
same duplication.
> I've got not idea, if we can just remove WB_DOCKABLE or if it should be
> replaced by another constant.
>
> Any advice ?
I think you can simply remove one of the duplicates.
--
Lubos Lunak
l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
[PUSHED] Re: Cppcheck reports "Same expression on both sides" in writerfilter module
Pushed on master (http://cgit.freedesktop.org/libreoffice/core/commit/?id=39ba666f802db07e34eb848ad4e5f39ff85dd6ed) (cf last post http://nabble.documentfoundation.org/Cppcheck-reports-Same-expression-on-both-sides-of-on-basctl-module-tp3893608p3895210.html) Thank you too for this one Lubos. Julien. -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Same-expression-on-both-sides-in-writerfilter-module-tp3893599p3895221.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
[SOLVED] Re: Question cppcheck reports on svx/source/svdraw/svdoedge.cxx (Bezier curve)
On 27/02/2013 00:20, Caolán McNamara wrote: On Wed, 2012-12-19 at 15:00 -0800, julien2412 wrote: Hello, Cppcheck detected this into svx/source/svdraw/svdoedge.cxx [svdoedge.cxx:1450]: (style) Variable 'pPt1' is assigned a value that is never used. [svdoedge.cxx:1453]: (style) Variable 'pPt4' is assigned a value that is never used That SdrEdgeObj::ImpCalcEdgeTrack is one of the most unpleasantly long and unreadable things I've seen in LibreOffice. Either pPt1 and pPt4 assignations may really be removed or a change must be done here. git blame&& show on results indicate it's there since initial OOo-ification in 2000. It looks like a dead loss to me to do anything except remove the assignations that have no utility. I pushed the fix on master (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=2e8d0fcad75d9b28c384ddac03bb45186694231f) Thank you! Julien ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports 'nMaxWidth' is assigned a value never used (svtools module)
On Tue, 2013-02-12 at 08:13 -0800, julien2412 wrote:
> 544 if( nMaxWidth > aSize.Width() )
> 545 {
> 546 Size aDlgSize = GetPathDialog()->GetOutputSizePixel();
> 547 GetPathDialog()->SetOutputSizePixel( Size(
> aDlgSize.Width()+nMaxWidth-aSize.Width(), aDlgSize.Height() ) );
> 548 aSize.Width() = nMaxWidth;
> 549
> 550 if( pOkBtn )
> 551 pOkBtn->SetSizePixel( aSize );
> 552 if( pCancelBtn )
> 553 pCancelBtn->SetSizePixel( aSize );
> 554 if( pLoadBtn )
> 555 pLoadBtn->SetSizePixel( aSize );
> 556 }
> 557 else
> 558 nMaxWidth = aSize.Width();
>
> see
> http://opengrok.libreoffice.org/xref/core/svtools/source/dialogs/filedlg2.cxx#544
>
> Any idea what should be done with this?
It's another "been like this since day 0", go ahead and remove the
does-nothing else branch. (The exciting manual move of widgets around
and determination of the size etc can all eventually go if it gets
converted to the widget layout stuff)
> I noticed these lines in tools/inc/tools/gen.hxx:
> longWidth() const { return nA; }
> long&Width() const { return nA; }
> I must recognize I don't understand the interest since there's a setWidth
> function
and setWidth just calls the second Width() variant. I can only speculate
it was an ancient abandoned incremental cleanup effort to remove one or
the other.
C.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Cppcheck reports "reassign before using" variable in sw/source/core/doc/tblafmt.cxx
Hello, Cppcheck detected this: [sw/source/core/doc/tblafmt.cxx:1227] -> [sw/source/core/doc/tblafmt.cxx:1234]: (performance) Variable 'bRet' is reassigned a value before the old one has been used. 1221 // Attention: We need to save a general Header here 1222 sal_uInt16 nVal = AUTOFORMAT_ID; 1223 rStream << nVal 1224 << (sal_uInt8)2 // Character count of the Header including this value 1225 << (sal_uInt8)GetStoreCharSet( ::osl_getThreadTextEncoding() ); 1226 1227 bRet = 0 == rStream.GetError(); 1228 1229 // Write this version number for all attributes 1230 m_pImpl->m_AutoFormats[0].GetBoxFmt(0).SaveVersionNo( 1231 rStream, AUTOFORMAT_FILE_VERSION); 1232 1233 rStream << static_cast(m_pImpl->m_AutoFormats.size() - 1); 1234 bRet = 0 == rStream.GetError(); see http://opengrok.libreoffice.org/xref/core/sw/source/core/doc/tblafmt.cxx#1215 What's the use of line 1227 if bRet isn't used before its new assignation line 1234? Should this line be removed because we're sure lines before will be ok or should something be added here? Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-reassign-before-using-variable-in-sw-source-core-doc-tblafmt-cxx-tp4025241.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
Cppcheck reports 'fViewSizeX'/fViewSizeY are assigned a value never used (drawingplayer part)
Hello,
Cppcheck reported this:
drawinglayer/source/primitive2d/sceneprimitive2d.cxx
196 unreadVariable style Variable 'fViewSizeX' is assigned a value that
is
never used.
197 unreadVariable style Variable 'fViewSizeY' is assigned a value that
is
never used.193 if(fReducedVisualisationFactor !=
1.0)
194 {
195 fReduceFactor *=
fReducedVisualisationFactor;
196 fViewSizeX *= fReducedVisualisationFactor;
197 fViewSizeY *= fReducedVisualisationFactor;
198 }
199 }
see
http://opengrok.libreoffice.org/xref/core/drawinglayer/source/primitive2d/sceneprimitive2d.cxx#192
Indeed, after these lines, fViewSizeX and fViewSizeY are no more used.
It's been like this since 2009-10-20 (see
http://cgit.freedesktop.org/libreoffice/core/commit/?id=2a0a3168f4354285e59dd79d4477a4199c2f773b)
Should we remove the lines 196 and 197 or is something lacking?
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-fViewSizeX-fViewSizeY-are-assigned-a-value-never-used-drawingplayer-part-tp4036929.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: Cppcheck reports an 2 assignments which isn't used in drawinglayer module
On Fri, 2012-12-28 at 09:28 -0800, julien2412 wrote: > Hello, > > Cppcheck reported these: > [drawinglayer/source/primitive2d/sceneprimitive2d.cxx:196]: (style) Variable > 'fViewSizeX' is assigned a value that is never used. > [drawinglayer/source/primitive2d/sceneprimitive2d.cxx:197]: (style) Variable > 'fViewSizeY' is assigned a value that is never used. ... > Indeed, fViewSizeX and fViewSizeY. this time I don't think they should be > removed, I rather think something is lacking. > > Any idea? So taking fViewSizeX lets have a look at what the history of these lines is. git log --follow -S fViewSizeX drawinglayer/source/primitive2d/sceneprimitive2d.cxx gives commit 2a0a3168f4354285e59dd79d4477a4199c2f773b Author: Armin Weiss Date: Tue Oct 20 15:36:32 2009 + #i105065# speedup 3D/FontWork commit 407f7ae14844d0e55b9f0f87a19a7208762b05ed Author: Armin Le Grand Date: Wed Oct 7 14:25:40 2009 +0200 #i105323# added FastPath for 3D scene HitTest for 3d CustomShapes by re-using the buffered last render result commit 2d634e6084547875a170d8350850110238b9e55c Author: Armin Le Grand Date: Tue Sep 29 15:35:35 2009 +0200 #i105323# speedup of 3D handling mostly for CustomShapes; HitTest and interactions optimized commit 57cc1366165559c8a958486c12af08790e6995f9 Author: Armin Weiss Date: Tue Feb 26 07:28:52 2008 + removals, OLE, optimizations for primitives commit 821e3b2b3026f21e396a8da3e152781a546ef5f5 Author: Armin Weiss Date: Thu Oct 19 09:40:02 2006 + #i39532# primitive To see what the file actually looked like at a given revision... git show 57cc1366165559c8a958486c12af08790e6995f9:drawinglayer/source/primitive2d/sceneprimitive2d.cxx git show 821e3b2b3026f21e396a8da3e152781a546ef5f5:drawinglayer/source/primitive2d/sceneprimitive2d.cxx So looking through those fViewSizeX became assigned a value that is never used with 57cc1366165559c8a958486c12af08790e6995f9 and every other commit just moved it around or adapted it to later changes. Seeing as this commit happened in 2008 and the rest of that commit looks reasonable I think you should go ahead and remove those never used variables. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[PUSHED] Re: Cppcheck reports "Same expression on both sides of '|'" on basctl module
Pushed on master (http://cgit.freedesktop.org/libreoffice/core/commit/?id=39ba666f802db07e34eb848ad4e5f39ff85dd6ed) (there are 2 fixes in this one). I don't know if it could be useful to push it on 3.5. Anyway, thank you Lubos. Julien. -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Same-expression-on-both-sides-of-on-basctl-module-tp3893608p3895210.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
[SOLVED] Re: Cppcheck reports an 2 assignments which isn't used in drawinglayer module
Thank you Caolán for your feedback. I pushed a fix on master (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=93b8b04ae0125a85d92ad0c414969c12e49b301e). But I didn't remove completely these variables since they're used. -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-an-2-assignments-which-isn-t-used-in-drawinglayer-module-tp4026316p4039584.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
[SOLVED] Re: Cppcheck reports 'nMaxWidth' is assigned a value never used (svtools module)
On 28/02/2013 12:58, Caolán McNamara wrote:
On Tue, 2013-02-12 at 08:13 -0800, julien2412 wrote:
544 if( nMaxWidth> aSize.Width() )
545 {
...
556 }
557 else
558 nMaxWidth = aSize.Width();
see
http://opengrok.libreoffice.org/xref/core/svtools/source/dialogs/filedlg2.cxx#544
Any idea what should be done with this?
It's another "been like this since day 0", go ahead and remove the
does-nothing else branch. (The exciting manual move of widgets around
and determination of the size etc can all eventually go if it gets
converted to the widget layout stuff)
I noticed these lines in tools/inc/tools/gen.hxx:
longWidth() const { return nA; }
long& Width() const { return nA; }
I must recognize I don't understand the interest since there's a setWidth
function
and setWidth just calls the second Width() variant. I can only speculate
it was an ancient abandoned incremental cleanup effort to remove one or
the other.
As usual, thank you for your feedback Caolán! :-)
I pushed a commit on master, see
http://cgit.freedesktop.org/libreoffice/core/commit/?id=9509f5c7923ebd9a95068dd7b9231af79080b6bf
Finally, I replaced Width() by setWidth method to make it clearer.
Julien
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "reassign before using" variable in sw/source/core/doc/tblafmt.cxx
On 20/12/12 00:26, julien2412 wrote: > Hello, > > Cppcheck detected this: > [sw/source/core/doc/tblafmt.cxx:1227] -> > [sw/source/core/doc/tblafmt.cxx:1234]: (performance) Variable 'bRet' is > reassigned a value before the old one has been used. >1221 // Attention: We need to save a general Header here >1222 sal_uInt16 nVal = AUTOFORMAT_ID; >1223 rStream << nVal >1224 << (sal_uInt8)2 // Character count of the Header > including this value >1225 << (sal_uInt8)GetStoreCharSet( > ::osl_getThreadTextEncoding() ); >1226 >1227 bRet = 0 == rStream.GetError(); >1228 >1229 // Write this version number for all attributes >1230 m_pImpl->m_AutoFormats[0].GetBoxFmt(0).SaveVersionNo( >1231 rStream, AUTOFORMAT_FILE_VERSION); >1232 >1233 rStream << > static_cast(m_pImpl->m_AutoFormats.size() - 1); >1234 bRet = 0 == rStream.GetError(); > > see > http://opengrok.libreoffice.org/xref/core/sw/source/core/doc/tblafmt.cxx#1215 > > What's the use of line 1227 if bRet isn't used before its new assignation > line 1234? > Should this line be removed because we're sure lines before will be ok or > should something be added here? the error handling there leaves a lot to be desired... ideally every assignment to bRet should be followed by a test "if (!bRet) return false;", i don't think it's a good idea to try to continue storing the file if something failed and then end up with a corrupt file. there are apparently other functions in that cxx file with similar problems. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "reassign before using" variable in sw/source/core/doc/tblafmt.cxx
Thank you again Michael for your feedback.
I know there are comments like FIXME, TODO in LO and certainly others but I
don't know if there has been some formalism (not too much since it could be
very quickly unproductive) about this.
So, since there are lots of tasks/bugs in LO, perhaps it could be useful to
put a little formalism there to help to manage priorities, we could use
Doxygen tag \todo
Eg:
/// \todo (priority {Low, High, To Define}), Some (optional) text
(it could be useful to add option sources too if exists:
"cppcheck/Clang/pep8/etc.")
Of course, it would be useful only if a basic report (with optionnaly some
stats) + detailed report (with hyperlinks) can be quickly generated from
Doxygen
Julien
PS : hope I was clear :-)
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-reassign-before-using-variable-in-sw-source-core-doc-tblafmt-cxx-tp4025241p4025379.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: Cppcheck reports "reassign before using" variable in sw/source/core/doc/tblafmt.cxx
On Thursday 20 of December 2012, julien2412 wrote:
> Thank you again Michael for your feedback.
>
> I know there are comments like FIXME, TODO in LO and certainly others but I
> don't know if there has been some formalism (not too much since it could
> be very quickly unproductive) about this.
>
> So, since there are lots of tasks/bugs in LO, perhaps it could be useful to
> put a little formalism there to help to manage priorities, we could use
> Doxygen tag \todo
> Eg:
> /// \todo (priority {Low, High, To Define}), last_author> Some (optional) text
> (it could be useful to add option sources too if exists:
> "cppcheck/Clang/pep8/etc.")
>
> Of course, it would be useful only if a basic report (with optionnaly some
> stats) + detailed report (with hyperlinks) can be quickly generated from
> Doxygen
I doubt that would be useful, because, well, what value would it actually
bring beside formalism? Somebody's high priority is somebody else's low
priority. We have version control system for knowing who and when changed
something, and commit log or comments for why. So it's not really different
from plain '// TODO '. Besides, I think we've been cleaning
up the codebase from things like this for the last 2 years.
So this solution first needs a problem it's supposed to solve, sorry.
--
Lubos Lunak
l.lu...@suse.cz
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Cppcheck reports an assignment which isn't used in sd/source/ui/func/fupage.cxx
Hello,
Cppcheck reported this:
[sd/source/ui/func/fupage.cxx:243]: (style) Variable 'pPtr' is assigned a
value that is never used
Here are some lines:
236 const sal_uInt16* pPtr = aNewAttr.GetRanges();
237 sal_uInt16 p1 = pPtr[0], p2 = pPtr[1];
238 while(pPtr[2] && (pPtr[2] - p2 == 1))
239 {
240 p2 = pPtr[3];
241 pPtr += 2;
242 }
243 pPtr += 2;
see
http://opengrok.libreoffice.org/xref/core/sd/source/ui/func/fupage.cxx#238
pPtr isn't used.
So either it can just be removed or something is wrong here.
Any idea?
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Cppcheck-reports-an-assignment-which-isn-t-used-in-sd-source-ui-func-fupage-cxx-tp4026217.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
[RESOLVED] Re: Cppcheck reports Resource leak in setup_native/source/win32/wintools/makecab/parseddf.c
On Thu, 2012-12-27 at 06:30 -0800, julien2412 wrote: > Hello, > > cppcheck reported this: > [setup_native/source/win32/wintools/makecab/parseddf.c:381]: (error) > Resource leak: ddf > it's in the function ParseDdf > fclose function seems to miss. Fixed by your own http://cgit.freedesktop.org/libreoffice/core/commit/?id=e7014ca47b9147c925fb99154b55c71c7127e935 now I believe. C. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Cppcheck reports "Deallocation of an auto-variable results in undefined behaviour" in vcl part
Hello, Cppcheck reported this: [vcl/source/gdi/bitmap4.cxx:229]: (error) Deallocation of an auto-variable results in undefined behaviour 137 long(*pKoeff)[ 256 ] = new long[ 9 ][ 256 ]; ... 229 delete[] pKoeff; See http://opengrok.libreoffice.org/xref/core/vcl/source/gdi/bitmap4.cxx#137 False positive? Moreover, I searched info about declaring 2D arrays and this notation new T[x][y] isn't supposed to work (see http://stackoverflow.com/questions/936687/how-do-i-declare-a-2d-array-in-c-using-new) Any idea? Julien -- View this message in context: http://nabble.documentfoundation.org/Cppcheck-reports-Deallocation-of-an-auto-variable-results-in-undefined-behaviour-in-vcl-part-tp4106524.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: Cppcheck reports an assignment which isn't used in sd/source/ui/func/fupage.cxx
Hello Julien,
but it is used, it is an array
*sal_uInt16 p1 = pPtr[0], p2 = pPtr[1];
while(pPtr[2] && (pPtr[2] - p2 == 1))*
On Thu, Dec 27, 2012 at 4:43 PM, julien2412 wrote:
> Hello,
>
> Cppcheck reported this:
> [sd/source/ui/func/fupage.cxx:243]: (style) Variable 'pPtr' is assigned a
> value that is never used
>
> Here are some lines:
> 236 const sal_uInt16* pPtr = aNewAttr.GetRanges();
> 237 sal_uInt16 p1 = pPtr[0], p2 = pPtr[1];
> 238 while(pPtr[2] && (pPtr[2] - p2 == 1))
> 239 {
> 240 p2 = pPtr[3];
> 241 pPtr += 2;
> 242 }
> 243 pPtr += 2;
> see
> http://opengrok.libreoffice.org/xref/core/sd/source/ui/func/fupage.cxx#238
> pPtr isn't used.
>
> So either it can just be removed or something is wrong here.
>
> Any idea?
>
> Julien
>
>
>
>
> --
> View this message in context:
> http://nabble.documentfoundation.org/Cppcheck-reports-an-assignment-which-isn-t-used-in-sd-source-ui-func-fupage-cxx-tp4026217.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
>
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports an assignment which isn't used in sd/source/ui/func/fupage.cxx
On 28/12/2012 12:37, Ioan Radu wrote: Hello Julien, but it is used, it is an array *sal_uInt16 p1 = pPtr[0], p2 = pPtr[1]; while(pPtr[2] && (pPtr[2] - p2 == 1))* Hi Loan, Yes but what about after line 243? What's the use of line 243 if "pPtr" isn't used afterwards? Julien ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports an assignment which isn't used in sd/source/ui/func/fupage.cxx
2012/12/28 Julien Nabet : > On 28/12/2012 12:37, Ioan Radu wrote: > > Hello Julien, > but it is used, it is an array > sal_uInt16 p1 = pPtr[0], p2 = pPtr[1]; > while(pPtr[2] && (pPtr[2] - p2 == 1)) > > Hi Loan, > > Yes but what about after line 243? What's the use of line 243 if "pPtr" > isn't used afterwards? > The assignment in line 243 is unnecessary. Allothers are still relevant. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PUSHED:master,3-5] Cppcheck reports "Logical disjunction always evaluates to true" in sdext module
julien2412 píše v Ne 18. 03. 2012 v 10:24 -0700: > Hello, > > Cppcheck reports this in sdext module : > [source/minimizer/optimizerdialog.cxx:276]: (warning) Logical disjunction > always evaluates to true: nNewStep >= 0 || nNewStep <= 4 > Here is the line : > if ( ( nNewStep != mnCurrentStep ) && ( ( nNewStep <= MAX_STEP ) || ( > nNewStep >= 0 ) ) ) > > Shouldn't it be : > if ( ( nNewStep != mnCurrentStep ) && ( nNewStep <= MAX_STEP ) && ( nNewStep > >= 0 ) ) Yup, it makes perfect sense. I have pushed the change into both master and 3-5 branch, see http://cgit.freedesktop.org/libreoffice/core/commit/?id=9481eb32e76568fc675982479cc07c57c6a7cb39 http://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-3-5&id=8eb5b36284c3a6c6535508a3f0651be1b5ada2dc > Moreover, if I'm right, should we additionnally throw an exception (which > one ?) if "nNewStep" is not between 0 and MAX_STEP I would leave it as is. The affected method is OptimizerDialog::SwitchPage. IMHO, it is better to do not switch pages instead of canceling the whole operation with an useless exception. Best Regards, Petr ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Cppcheck reports "Deallocation of an auto-variable results in undefined behaviour" in vcl part
On Saturday 26 of April 2014, julien2412 wrote: > Hello, > > Cppcheck reported this: > [vcl/source/gdi/bitmap4.cxx:229]: (error) Deallocation of an auto-variable > results in undefined behaviour > 137 long(*pKoeff)[ 256 ] = new long[ 9 ][ 256 ]; > ... > 229 delete[] pKoeff; > See > http://opengrok.libreoffice.org/xref/core/vcl/source/gdi/bitmap4.cxx#137 > > False positive? Yes, it looks bogus. > Moreover, I searched info about declaring 2D arrays and this notation new > T[x][y] isn't supposed to work (see > http://stackoverflow.com/questions/936687/how-do-i-declare-a-2d-array-in-c- >using-new) Any idea? The sizes are constants, there's no such problem. -- Lubos Lunak l.lu...@collabora.com ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[RESOLVED] Re: Cppcheck reports an assignment which isn't used in sd/source/ui/func/fupage.cxx
On 28/12/2012 14:43, Markus Mohrhard wrote: 2012/12/28 Julien Nabet: On 28/12/2012 12:37, Ioan Radu wrote: Hello Julien, but it is used, it is an array sal_uInt16 p1 = pPtr[0], p2 = pPtr[1]; while(pPtr[2]&& (pPtr[2] - p2 == 1)) Hi Loan, Yes but what about after line 243? What's the use of line 243 if "pPtr" isn't used afterwards? The assignment in line 243 is unnecessary. Allothers are still relevant. Thank you Markus, I removed it. (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=ffe1a2a92236a9b918b5c31d03d04332f2c63876). Julien ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
