Re: About writeDBLoaderInfo function in dbloader.cxx (dbaccess module)

On Sat, Aug 31, 2013 at 02:26:43PM -0700, julien2412 [via Document Foundation Mail Archive] wrote:

> cppcheck reported this:
> dbaccess/source/ui/browser/dbloader.cxx
> 167   redundantAssignment     style   Variable 'xNewKey' is reassigned a value
> before the old one has been used

> Here's the function:
>     153 extern "C" void SAL_CALL writeDBLoaderInfo(void* pRegistryKey)
>     154 {
>     155     Reference< XRegistryKey> xKey(reinterpret_cast< XRegistryKey*>(pRegistryKey));
>     156 
>     157     // register content loader for dispatch
>     158     OUString aImpl("/");
>     159     aImpl += DBContentLoader::getImplementationName_Static();
>     160 
>     161     OUString aImpltwo = aImpl;
>     162     aImpltwo += "/UNO/Loader";
>     163     Reference< XRegistryKey> xNewKey = xKey->createKey( aImpltwo );
>     164     aImpltwo = aImpl;
>     165     aImpltwo += "/Loader";
>     166     Reference< XRegistryKey >  xLoaderKey = xKey->createKey( aImpltwo );
>     167     xNewKey = xLoaderKey->createKey( OUString("Pattern") );
>     168     xNewKey->setAsciiValue( OUString(".component:DB*") );
>     169 }
> (see
> http://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/browser/dbloader.cxx#153)

> Indeed, xNewKey is reassigned. But now I don't know how it should be
> changed.

The general "mechanical" rule is:

 - if the function whose return value populates the variable has a
   side effect, then just remove the
   Reference< XRegistryKey> xNewKey =

 - else, remove the whole line (and in this case, the string
   construction that prepares the argument to the function call).

"Side effect" means that the function has another function than
constructing its return value; that is, it changes the state of the
program in a way that after the return value is destroyed, still has
an effect.

In this particular case, it seems to me that it has a side effect,
namely creating the said configuration / registry key: if some other
code subsequently calls "openKey", they will get non-NULL, whereas
without that createKey() they will get NULL. The structure of the code
(with a aImpl and aImpltwo that gets reused) also looks to me like
this is done on purpose (as opposed to a copy/paste error).

On the other hand, it seems unusual to create an "empty" key just so
that future code will find it... And I don't find any other instance
of the string "UNO/Loader" anywhere in the code. Based on this
observation, I would remove the whole "createKey".

On the gripping hand, I can't find where the "//Loader" key is used
afterwards either, so maybe my search-fu simply failed me. Or maybe
the whole function is pointless. As an added bonus remark, it seems
weird (but maybe innocuous) that it goes to extra pains to create
"//UNO/Loader" and "//Loader" as opposed to "/UNO/Loader" and
"/Loader".

And searching more, I see that this function is actually never
called... It is not in any header file of the sdk (not in _any_ header
actually), so it can't be part of our public API (nor of our internal
API).

So actually, in this case, remove the whole function. And also the
function of the same name in dbaccess/source/filter/xml/dbloader2.cxx

The commit that removed its usage is:

commit 8e88ac109dc9eba88db92940d13933fc3a4393d8
Author: sb <sb@openoffice.org>
Date:   Fri Sep 10 13:10:07 2010 +0200

    sb129: #i113189# change UNO components to use passive registration

-- 
Lionel




--
View this message in context: 
http://nabble.documentfoundation.org/About-writeDBLoaderInfo-function-in-dbloader-cxx-dbaccess-module-tp4072511p4072521.html
Sent from the Dev mailing list archive at Nabble.com.