Re: [Libreoffice] [PATCH] [PUSHED, partial] Some comments and basic cleanup in sc

Hi Soeren,

On Sun, 2011-01-23 at 21:01 +0100, Soeren Moeller wrote:
> Hi
>
> Attached four patches:
> 0001: adding comments to ScFunctionMgr and ScFunctionList in sc/inc/funcdesc.hxx

-  e.g. functions used in formulas in calc
+  e.g.\ functions used in formulas in calc

I personally find this change a step backward, as this would reduce
readability in the source code itself.  Let's not this picky about
whitespace size (assuming that this escaping of the whilespace is used
for that reason).

-    ::rtl::OUString      *pFuncName;              // Function name
+    ::rtl::OUString      *pFuncName;              /**< Function name */
...

Here, you can simply turn // Function name into /// Function name to
make it doxygen compliant.  I find /**< .... */ (with the '<') a bit
ugly, and again this would reduce readability of the comment in the
source code.  There are code that already uses /// Foo type comments, so
let's stick with that style.

BTW does doxygen really treat /**< foo */ type comment in any special
way?  I looked at the doxygen manual, but there is no mention of /**<
foo */ type comment.  Or did you mean to type /** foo */ type comment
instead?

> 0002: removing duplicate implementation of
> ScFunctionMgr::fillLastRecentlyUsedFunctions

Looks good, though I made the following change:

diff --git a/sc/source/ui/formdlg/dwfunctr.cxx b/sc/source/ui/formdlg/dwfunctr.cxx
index 3175e9a..90e0706 100644
--- a/sc/source/ui/formdlg/dwfunctr.cxx
+++ b/sc/source/ui/formdlg/dwfunctr.cxx
@@ -816,11 +816,11 @@ void ScFunctionDockWin::UpdateFunctionList()
     }
     else // LRU-Liste
     {
-        for(::std::vector<const formula::IFunctionDescription*>::iterator 
iter=aLRUList.begin();iter!=aLRUList.end();iter++)
+        for(::std::vector<const formula::IFunctionDescription*>::iterator 
iter=aLRUList.begin();iter!=aLRUList.end();++iter)
         {
-            const ScFuncDesc* pDesc = static_cast<const ScFuncDesc*>(*iter);
+            const formula::IFunctionDescription* pDesc = *iter;
             pAllFuncList->SetEntryData(
-                    pAllFuncList->InsertEntry( *(pDesc->pFuncName) ),
+                    pAllFuncList->InsertEntry(pDesc->getFunctionName()),
                     (void*)pDesc );
         }
     }

When using iterator, it's always better to use pre-increment ++iter as
opposed to post-increment iter++ *unless* the behavior of post-increment
is called for.  That is not the case in this instance.

Also, you can get the function name string via IFunctionDescription's
getFunctionName() call, which eliminates the static_cast to ScFuncDesc.
It's cleaner this way.

This patch is now pushed to master.

> 0003: adding comments to ScFunctionCategory in sc/inc/funcdesc.hxx

This doesn't apply for some reason....  Perhaps depending on 0001?

Also, the same comment as in 0001 applies.  Let's not use /**< foo */
type comment.

> 0004: cleaning up comments and spacing, and minor translations in
> sc/source/core/data/funcdesc.cxx

Applied.  Pushed to master.

Could you revise 0001 and 0003 and resend them?  Thanks!

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kyoshida@novell.com>