Issue 114412 - sw: dubious delete in SwCalc::Str2Double
Summary: sw: dubious delete in SwCalc::Str2Double
Status: CLOSED FIXED
Alias: None
Product: Writer
Classification: Application
Component: code (show other issues)
Version: DEV300m87
Hardware: All All
: P3 Trivial (vote)
Target Milestone: 3.4.0
Assignee: dtardon
QA Contact: issues@sw
URL:
Keywords:
Depends on: 114409
Blocks:
  Show dependency tree
 
Reported: 2010-09-08 14:25 UTC by dtardon
Modified: 2017-05-20 10:22 UTC (History)
2 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---

Attachments
remove the delete and simplify the code a bit (4.02 KB, text/plain)
2010-09-08 14:26 UTC, dtardon 		no flags Details
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this issue.
Description dtardon 2010-09-08 14:25:12 UTC 
There is the following code in BOOL SwCalc::Str2Double( const String& rCommand,
xub_StrLen& rCommandPos, double& rVal, const LocaleDataWrapper* const pLclData ) :

if( !pLclData && pLclD != &SvtSysLocale.GetLocaleData() )
        delete (LocaleDataWrapper*)pLclD;

. The condition is always false because pLclData is assigned from
&SvtSysLocale::GetLocaleData() if it's 0. I've removed it and refactored the
remaining code a bit to get rid of duplicate code. The patch is only applicable
if the patch in #i114409# is also applied.
Comment 1 dtardon 2010-09-08 14:26:54 UTC 
Created attachment 71615 [details]
remove the delete and simplify the code a bit
Comment 2 andreas.martens 2010-09-08 15:32:27 UTC 
.
Comment 3 andreas.martens 2010-09-08 15:34:08 UTC 
.
Comment 4 andreas.martens 2010-09-08 15:35:01 UTC 
.
Comment 5 mst.ooo 2010-09-09 14:54:54 UTC 
@dtardon:

you've left the "if (...) delete pLclD" in the second copy of Str2Double,
can't see a reason why only one should be removed, i guess that's an oversight?
Comment 6 dtardon 2010-09-09 17:58:31 UTC 
dtardon->mst: because pLclD might have been created inside the function: see
pLclD = new LocaleDataWrapper(...) a few lines above. (I don't know if that
block is still relevant, though. Maybe the condition eLang !=
SvxLocaleToLanguage( pLclD->getLocale() ) is always false...)
Comment 7 mst.ooo 2010-09-09 18:04:02 UTC 
@dtardon:

d'oh, of course, now that you mention it i also see it :)
Comment 8 mst.ooo 2010-09-09 19:38:29 UTC 
thanks for removing the code duplication;
you have unfortunately introduced a problem that you never expected:
the stupid SunStudio C++ compiler can't link sw anymore, because it's stupid
and mangles the "const" that you added to the parameter into the method name,
which is afaik not allowed by the C++ standard (for good reasons,
because whether a parameter is const/volatile or not is purely an implementation
detail).
unfortunately it's necessary to adapt the header as well to make stupid Sun C++
happy.

so i've fixed that and then refactored a bit more to replace the UGLY BOOL with
nice bool and
also replaced the other delete with an auto_ptr. 

fixed in cws sw34bf01
http://hg.services.openoffice.org/hg/cws/sw34bf01/rev/43f603d097e0
http://hg.services.openoffice.org/hg/cws/sw34bf01/rev/6e6683f194c5
Comment 9 mst.ooo 2010-10-07 11:19:45 UTC 
please verify
Comment 10 dtardon 2010-10-07 20:32:10 UTC 
looks good
Comment 11 caolanm 2010-10-13 20:28:15 UTC 
um, http://hg.services.openoffice.org/hg/cws/sw34bf01/rev/6e6683f194c5 doesn't
make any sense to me.

::std::auto_ptr<const LocaleDataWrapper> pLclD;
...
if( eLang != SvxLocaleToLanguage( pLclD->getLocale() ) )
...
i.e. dereference of unset pLclD

maybe, 
if( eLang != SvxLocaleToLanguage( aSysLocale.GetLocaleData().getLocale() ) )
is what was intended
Comment 12 mst.ooo 2010-10-14 13:18:28 UTC 
@cmc:

d'oh!
of course you are right!
114412 is now officially my unlucky number...

fixed here (but please double-check it):
http://hg.services.openoffice.org/hg/cws/sw34bf01/rev/2b2b31b4f97e
Comment 13 caolanm 2010-10-14 13:24:02 UTC 
That seems sane to me, yup