Date: prev next · Thread: first prev next last
2011 Archives by date, by thread · List index


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>


Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.