Re: [Libreoffice] Splitting up source/header files for clarity
I have now splitted the implementation of funcdesc.hxx out into a new source file funcdesc.cxx. In the process I also have extracted two new header files for classes previously defined in global.cxx. I have corrected all includes and the result compiles for me, I have added the new source file to the relevant makefile.mk. Further I have added comments to the first class in funcdesc.hxx, I hope to document the others in a later patch. Please review my patch and apply if ok. Best Regards Sören Möller (LGPLv3+ / MPL) 2011/1/10 Tor Lillqvist tlillqv...@novell.com: - Is it best to make one file for each class, or keep them together and make one funcdesc.cxx (containing four classes)? I'd say one file, funcdesc.cxx, corresponding to funcdesc.hxx. But that is just my personal opinion. - Should the license header for the new file(s) be a copy of the license header from global.cxx (as the code is copied out), or the new Libo header? As the code is the same (just rearranged into different files), the license header should be the same. - Is it enough to add the new file(s) to sc/source/core/data/makefile.mk for building, Yes, that should be enough for your testing; if/when you then want to make a patch out of it, you need to git add the new files to your local repository clone.) --tml ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Splitting up source/header files for clarity
Soeren Moeller wrote:
I have now splitted the implementation of funcdesc.hxx out into a new
source file funcdesc.cxx.
Hi Soeren,
first off, many thanks for going to clean this up, your work here is
much appreciated! That split so far looks good.
In the process I also have extracted two new header files for
classes previously defined in global.cxx.
That's not necessary - ScResourcePublisher and ScFuncRes are only
used inside global.cxx, thus need no extra header - in fact, this
would not be idiomatic for c++. Simply leave them next to the code
that's using them.
See a few more comments inline of the actual patch:
diff --git a/sc/inc/funcdesc.hxx b/sc/inc/funcdesc.hxx
index d94d9b2..5f3ca7e 100644
--- a/sc/inc/funcdesc.hxx
+++ b/sc/inc/funcdesc.hxx
@@ -34,39 +34,161 @@
* global.cxx
*/
+#include scfuncs.hrc
+
#include tools/list.hxx
-#include tools/string.hxx
+//#include tools/string.hxx
Please remove, don't comment out.
+/**
+ Contains description of a function
+*/
class ScFuncDesc : public formula::IFunctionDescription
{
public:
+/**
+ Constructor
+*/
+ScFuncDesc();
-virtual ::rtl::OUString getFunctionName() const ;
+/**
+ Destructor
+*/
+virtual ~ScFuncDesc();
Those doc-comments don't provide any value. Destructors and nullary
constructors seldomly need any comment at all, and for the class,
I'd suggest something along the lines of Serves human-language
function descriptions within the calc formula parser framework - or
somesuch. Did not look too closely, TBH. ;)
Two related golden rules for good comments/documentation:
Good comments don't repeat the code or explain it. They clarify its
intent. Comments should explain, at a higher level of abstraction
than the code, what it does.
or, put bluntly:
Don’t insult the reader’s intelligence ;)
+/**
+ Resets the object in a clean manner
+*/
+void Clear();
For one-liners like this, I'd prefer this markup:
/// Resets the object as if it were newly constructed
(well, does clear() perform that? writing good comments usually
involves some detailed code review, or even debugging)
+/**
+ Returns the category of the function
+
+ @returnthe category of the function
+*/
virtual const formula::IFunctionCategory* getCategory() const ;
Since these methods are inherited from the IFunctionDescription
interface, I'd rather document them there (in the formula module).
That way, all derived classes benefit from your documentation (and
doxygen is smart enough to link that).
+/* Public variables */
I'd consider this redundant. If you want to comment on the fact that
this class exposes its data structures, try to reconstruct the
reason why this is so - e.g. a bug number, for which it was added,
e.g. in a hurry before a shipment, to keep changes minimal (and
later cleanup was forgotten). Else, the reason would be programmer
lazyness / sloppy coding, and that's a default assumption that
needs no comment. ;)
-USHORTnFIndex;// Unique function index
-USHORTnCategory; // Function category
-USHORTnArgCount; // All parameter count,
suppressed and unsuppressed
-USHORTnHelpId;// HelpID of function
+sal_uInt16nFIndex;// Unique function index
+sal_uInt16nCategory; // Function category
+sal_uInt16nArgCount; // All parameter count,
suppressed and unsuppressed
+sal_uInt16nHelpId;// HelpId of function
Ok to change, but I'd then also adapt the other occurences. And
maybe make that a separate patch - makes review easier, and ensures
that uncontroversial changes get in right away. :)
Could you quickly brush this up a bit resend? As I said, otherwise
good useful!
Cheers,
-- Thorsten
pgpGWEo7M0tbf.pgp
Description: PGP signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Splitting up source/header files for clarity
- Is it best to make one file for each class, or keep them together and make one funcdesc.cxx (containing four classes)? I'd say one file, funcdesc.cxx, corresponding to funcdesc.hxx. But that is just my personal opinion. - Should the license header for the new file(s) be a copy of the license header from global.cxx (as the code is copied out), or the new Libo header? As the code is the same (just rearranged into different files), the license header should be the same. - Is it enough to add the new file(s) to sc/source/core/data/makefile.mk for building, Yes, that should be enough for your testing; if/when you then want to make a patch out of it, you need to git add the new files to your local repository clone.) --tml ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] Splitting up source/header files for clarity
Hi I have been working with sc/inc/funcdesc.hxx, and this header defines four closely related classes, but all the implementation of these classes is in sc/source/core/data/global.cxx together with the implementation of a number of other classes. This makes it confusing to find the implementations and check these classes separately, so it would be nice to have these implementations in their own file/files. This is even suggested by a comment in sc/inc/funcdesc.hxx which sounds, as if the header earlier was part of some huge file too. Moving these implementations to own files raises some questions: - Is it best to make one file for each class, or keep them together and make one funcdesc.cxx (containing four classes)? - Should the license header for the new file(s) be a copy of the license header from global.cxx (as the code is copied out), or the new Libo header? - Is it enough to add the new file(s) to sc/source/core/data/makefile.mk for building, or have they to be added somewhere else too? Any comments are appreciated Best Regards Sören Möller ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
