[Libreoffice] Script to find undocumented classes
Hi, I'm attaching a patch that adds a script to the scratch directory of a build repo that tries to find undocumented classes using doxygen. It's meant to be invoked in a subdirectory of the source code, so in case one is familiar with Calc, she can run it under sc/ and add doxygen comments to the classes she know, etc. I'm intentionally not using 'make docs', because the target here is to allow quick iteration, ie. to run 'find-undocumented-classes.sh -q' in a subdirectory, add a few comments, run again, etc. Does the idea sounds sane, OK to push? Thanks. From 0fd0d400e3a64cb9619a207868c835d18acce162 Mon Sep 17 00:00:00 2001 From: Miklos Vajna Date: Mon, 22 Nov 2010 14:33:57 +0100 Subject: [PATCH] Add script to find undocumented classes --- scratch/find-undocumented-classes.sh | 26 ++ 1 files changed, 26 insertions(+), 0 deletions(-) create mode 100755 scratch/find-undocumented-classes.sh diff --git a/scratch/find-undocumented-classes.sh b/scratch/find-undocumented-classes.sh new file mode 100755 index 000..58a133b --- /dev/null +++ b/scratch/find-undocumented-classes.sh @@ -0,0 +1,26 @@ +#!/bin/bash + +# finds undocumented classes in the current directory (recursive) + +filter= +quiet=n +if [ "$1" = "-q" ]; then +filter=">/dev/null" +quiet=y +fi + +doxygen=$(mktemp -d) +eval doxygen -g $doxygen/doxygen.cfg $filter +sed -i "/HTML_OUTPUT/s|html|$doxygen/html|" $doxygen/doxygen.cfg +sed -i '/GENERATE_LATEX/s/= YES/= NO/' $doxygen/doxygen.cfg +sed -i '/RECURSIVE/s/= NO/= YES/' $doxygen/doxygen.cfg +eval doxygen $doxygen/doxygen.cfg $filter 2> $doxygen/errors.txt +if [ "$quiet" == "n" ]; then +echo +echo "The following classes are undocumented:" +echo +fi +cat $doxygen/errors.txt|grep 'Warning: Compound.*is not documented' +rm -rf $doxygen + +# vim:set shiftwidth=4 softtabstop=4 expandtab: -- 1.7.3.2 pgpH5wsBDaPkn.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Miklos Vajna wrote: > Does the idea sounds sane, OK to push? > Totally! Would you maybe also be interested to work on an improved start page / index, e.g. like this here: http://www.informatik.uni-hamburg.de/~meine/hg/vigra/file/bf402d01f821/docsrc/index.dxx (see http://www.informatik.uni-hamburg.de/~meine/hg/vigra/file/bf402d01f821/include/vigra/matrix.hxx for how defgroup / ingroup etc. could be used)? Additionally, I think most classes don't necessarily need detailed docs for all methods in the first place (which may also hurt later merging from OOo), but would already benefit from a two-line "mission statement" at class level (of course plus some module-level overview of "what's inside"). Cheers, -- Thorsten pgppVNIlYFzxW.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Tuesday 30 of November 2010, Thorsten Behrens wrote: > Additionally, I think most classes don't necessarily need detailed > docs for all methods in the first place (which may also hurt later > merging from OOo), but would already benefit from a two-line > "mission statement" at class level (of course plus some module-level > overview of "what's inside"). I beg to differ. After having years of experience using a nice, intuitive and well-documented APIs (Qt,KDE), and being used to that, I sometimes rather suffer getting familiar with this codebase. Most APIs are not documented at all (or at most poorly, or in German, which is about the same in practice for many people). This is futher made worse by some APIs not being very intuitive (cryptic abbreviations, unclear naming, obsolete idiosyncracies, duplication, basic things being needlessly complicated). This could be a significant factor for new possible contributors. While patches removing dead code or similar certainly help too, the codebase can move forward only by people writing new code, and that requires understanding of the existing code. So I find any suggesting that docs are not necessary (or valueing merging higher) to be eventually shooting ourselves in the foot. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Tue, Nov 30, 2010 at 04:21:08PM +0100, Thorsten Behrens wrote: > Miklos Vajna wrote: > > Does the idea sounds sane, OK to push? > > > Totally! OK, pushed. > Would you maybe also be interested to work on an improved > start page / index, e.g. like this here: > > http://www.informatik.uni-hamburg.de/~meine/hg/vigra/file/bf402d01f821/docsrc/index.dxx > > (see > http://www.informatik.uni-hamburg.de/~meine/hg/vigra/file/bf402d01f821/include/vigra/matrix.hxx > for how defgroup / ingroup etc. could be used)? Hm, sure - but I guess that in general needs a general knowledge of at least a whole module, which is not really the case for me. Or you mean a group for each subdirectory, which is probably something I can do? :) > Additionally, I think most classes don't necessarily need detailed > docs for all methods in the first place (which may also hurt later > merging from OOo), but would already benefit from a two-line > "mission statement" at class level (of course plus some module-level > overview of "what's inside"). Yes, I think it's more important to have a mission statement for a lot of classes than a few fully documented ones. :) That's what the script does - normally doxygen emits tons of warnings and this way it's easy to search for classes where there isn't a single line of doxygen comment at all. OTOH in the long run I agree with Lubos that it would be nice to have methods documented as well, unless that makes merging really hard. (I don't think it makes merging harder than comment cleanup does.) pgpmM6ovrCaE8.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Lubos Lunak wrote: > On Tuesday 30 of November 2010, Thorsten Behrens wrote: > > Additionally, I think most classes don't necessarily need detailed > > docs for all methods in the first place (which may also hurt later > > merging from OOo), but would already benefit from a two-line > > "mission statement" at class level (of course plus some module-level > > overview of "what's inside"). > > I beg to differ. After having years of experience using a nice, intuitive > and > well-documented APIs (Qt,KDE), and being used to that, I sometimes rather > suffer getting familiar with this codebase. Most APIs are not documented at > all (or at most poorly, or in German, which is about the same in practice for > many people). This is futher made worse by some APIs not being very intuitive > (cryptic abbreviations, unclear naming, obsolete idiosyncracies, duplication, > basic things being needlessly complicated). > Hi Lubos, adding documentation for what you describe doesn't help much, if at all. Core API is reasonably documented, see offapi/udkapi/sal/basegfx. The most egregious cases of hard-to-grasp methods surely live in the applications cores, use lots of weird abbreviations - and to document their behaviour succinctly would prolly require as much prose as there's code already inside them (i.e. multiple pages). For me, a cleanly designed class with one and only one task (and a proper mission statement, maybe - superfluous even for something like "String"), does not need much method documentation. Suitably-chosen method names, especially if they don't take 10 parameters, and only work for exactly 42 combinations of them, are almost self-documenting. I simply refuse the notion that adding superficial documentation to crappy api gets us anywhere - which does not rule out storing hard-won knowledge of class purpose, inner workings, important pre- or postconditions at times - and if it makes sense, as method- or class-level doxygen documentation. But to really fix the problem you mention, I'm afraid only large-scale refactoring helps. And wrong documentation surely is worse than no documentation. (sorry for the rant, that struck a chord. Also, I'm clearly with you that larger parts of vcl & [sv]tools public headers are in dire need of method-level documentation. I'd even suggest to start there, that code does not change much) Cheers, -- Thorsten pgpxbxY0c6BKk.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Tue, 30 Nov 2010 18:50:17 +0100, Lubos Lunak wrote: > On Tuesday 30 of November 2010, Thorsten Behrens wrote: > > Additionally, I think most classes don't necessarily need detailed > > docs for all methods in the first place (which may also hurt later > > merging from OOo), but would already benefit from a two-line > > "mission statement" at class level (of course plus some module-level > > overview of "what's inside"). > > I beg to differ. > This could be a significant factor for new possible contributors. While > patches removing dead code or similar certainly help too, the codebase can > move forward only by people writing new code, and that requires understanding > of the existing code. I am with Lubos here I have to say. I do see Thorsten's point on that wrong documentation is worse than no documentation, and fixing wrong APIs and function names would be preferred. I also see how it makes merging worse. BUT, as a newcomers, I was very much overwhelmed with millions of lines of code, many of which were not documented at all. And as a newcomer you don't even know what is a core API and what is a seldomly used thing. People who are not intimate with but rather intimidated by the code base, do need these docs to steer around and understand the code. Being able to look up in doxygen what a "bFull" parameter implies and which effect it has in a function, would be a real boon. Let me give a random real world example that I just pulled up by searching the code base for a bFull parameter :): In /libs-core/editeng/inc/editeng/unofored.hxx we define: QuickFormatDoc( BOOL bFull=FALSE ); What does bFull mean? Not so quick? What portions will be formatted if this is FALSE? Looking at the function it either calls pImpEditEngine->FormatFullDoc(); or pImpEditEngine->FormatDoc(); What the heck is the difference between those functions? Now I have to go another layer deeper to those 2 (both undocumented!) functions. All that FormatFullDoc does: for ( sal_uInt16 nPortion = 0; nPortion < GetParaPortions().Count(); nPortion++ ) GetParaPortions()[nPortion]->MarkSelectionInvalid( 0, GetParaPortions()[nPortion]->GetNode()->Len() ); FormatDoc() Excuse me, what does the stuff before we end up in FormatDoc() actually do? It must modify the document somehow as a sideeffect,because we call FormatDoc without any parameter. And it seems to mark some selections as invalid. So perhaps bFull=FALSE only works on selected text? For that I need to dig into what GetParaPortions and ParaPortions actually are and do, which is an *undocumented class* implemented here: libs-core/editeng/source/editeng/editdoc2.cxx. I'd give up at this point, because after reading that much code, I had forgotten what I wanted to do in the first place :). A simple docstring in QuickFormatDoc, such as /** * param bFull determines whether we need to reflow the whole document or only the pieces that are visible on the screen. */ would have saved me much time, and I could actually have improved some code rather becoming a frustrated opengrok hunter.. (note this is a bullshit comment, as I *still* don't know what bFull really does :)). Sebastian pgpAGSX9awKLR.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
> After having years of experience using a nice, intuitive > and well-documented APIs (Qt,KDE), But surely you should realize the causality here? It is exactly your years of experience with them that makes Qt and KDE seem nice and intuitive. It is extremely hard, probably impossible, to come up with an objective way to indicate what is intuitive. As the old saying goes, the only intuitive thing (for a mammal) is the nipple. Everything else is learned. The same argumentation is seen over and over again in all areas of software... "I have used Photoshop for years, it is so intuitive, GIMP sucks." "I have used Windows for years, it is so intuitive, Linux sucks." (Intentionally just saying "Linux" here because people who make that claim hardly know which FLOSS desktop environment they are referring to.) "I have used Fortran for years, it is so intuitive for numerical programming." I am sure it is not hard to find people who think the OOo code APIs are elegant, consistent and intuitive. And they would be equally right. > So I find any suggesting that docs are not necessary (or valueing merging > higher) to be eventually shooting ourselves in the foot. Personally I think it would be very nice if source files had just a few lines of comment telling what they are about, from a very high perspective. Every time I am searching for the implementation of some functionality in the OOo/LO codebase, I find myself looking at source files with vaguely correct sounding file names (and vaguely correctly sounding class names inside), but no clue if it actually is what I am looking for. (Yeah, now that we have forked off the codebase, I of course should take up the habit of adding such a few comment lines every time I find out what the code in some particular source file does, especially in cases where one can easily be mislead and spend hours looking at irrelevant code before realizing...) --tml ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Sebastian Spaeth wrote: > What does bFull mean? Not so quick? What portions will be formatted if > this is FALSE? Looking at the function it either calls > pImpEditEngine->FormatFullDoc(); > or > pImpEditEngine->FormatDoc(); > > What the heck is the difference between those functions? Now I have to > go another layer deeper to those 2 (both undocumented!) functions. > > All that FormatFullDoc does: > > for ( sal_uInt16 nPortion = 0; nPortion < GetParaPortions().Count(); > nPortion++ ) > GetParaPortions()[nPortion]->MarkSelectionInvalid( 0, > GetParaPortions()[nPortion]->GetNode()->Len() ); > FormatDoc() > > Excuse me, what does the stuff before we end up in FormatDoc() actually > do? It must modify the document somehow as a sideeffect,because we call > FormatDoc without any parameter. And it seems to mark some selections as > invalid. So perhaps bFull=FALSE only works on selected text? > > For that I need to dig into what GetParaPortions and ParaPortions > actually are and do, which is an *undocumented class* implemented here: > libs-core/editeng/source/editeng/editdoc2.cxx. > > I'd give up at this point, because after reading that much code, I had > forgotten what I wanted to do in the first place :). A simple docstring > in QuickFormatDoc, such as > /** > * param bFull determines whether we need to reflow the whole document > or only the pieces that are visible on the screen. > */ > Hi spaetz, see? That's what I meant, documentation for most of the higher-level methods is either a) superficial b) so much prose that you're better off debugging the code in the first place (bFull has *a lot* of side effects, and no, I did not bother to research all of them for the while) > would have saved me much time, and I could actually have improved some > code rather becoming a frustrated opengrok hunter.. > (note this is a bullshit comment, as I *still* don't know what bFull > really does :)). > Having a ctags and/or idutils index setup & integrated into your editor greatly speeds up jumping around in the codebase. I'd consider it essential for any productive work with LibO. Cheers, -- Thorsten pgpoX6fSo7N6y.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Wednesday 01 of December 2010, Tor Lillqvist wrote: > > After having years of experience using a nice, intuitive > > and well-documented APIs (Qt,KDE), > > But surely you should realize the causality here? It is exactly your years > of experience with them that makes Qt and KDE seem nice and intuitive. > > It is extremely hard, probably impossible, to come up with an objective way > to indicate what is intuitive. As the old saying goes, the only intuitive > thing (for a mammal) is the nipple. Everything else is learned. > > The same argumentation is seen over and over again in all areas of > software... "I have used Photoshop for years, it is so intuitive, GIMP > sucks." "I have used Windows for years, it is so intuitive, Linux sucks." > (Intentionally just saying "Linux" here because people who make that claim > hardly know which FLOSS desktop environment they are referring to.) "I have > used Fortran for years, it is so intuitive for numerical programming." > > I am sure it is not hard to find people who think the OOo code APIs are > elegant, consistent and intuitive. And they would be equally right. Well, no, sorry. First of all, while most of my experience comes from working with Qt/KDE, I usually worked on the lower level parts of it, making me often look at other software, be it libs like fontconfig, Xlib, polkit, or various non-KDE software. So I do have quite some experience figuring out unknown code. Second, I think it is possible to measure how good API/documentation is, to some extent. You can compare how extensively and well it is documented (sure, there are some exceptions like polkit being very extensively and rather unhelpfully documented, but generally it's rather hard to beat something like e.g. [1]). You can look at some code and see whether you can guess the meaning even without being familiar with it ("const SwFmtFrmSize& rLSz = pLineFmt->GetFrmSize();" - huh? I still need to decipher this one and I've been told the meaning at least 3 times). You can look at the documentation and see how quickly you find a way to do something you need. And so on. I'm not going to dwell much on showing that Qt/KDE APIs are easier to use, because quite frankly I find that obvious, you can just ask anybody around or try it yourself. They are simply designed that way and there are certain requirements, while it seems LibO doesn't have that. I may be biased because of familiarity, but the difference is simply huge. And BTW, [2] is a very interesting read. I don't mean to bash anybody for this, but this is simply the biggest impression with LibO that I have - that it's awfully difficult to get into it. The combination of massive codebase and poorly documented (and often cryptic) APIs is just overwhelming. I may have time to get over it, but does that mean we're not interested in those possible contributors who wouldn't have this much time and guidance? Speaking of which, since the opening of LibO, how many of those new contributors have contributed something that requires at least some understanding of the codebase (i.e. not counting dead code removal, comment translations or similar)? [1] http://doc.qt.nokia.com/4.7/qstring.html [2] http://doc.trolltech.com/qq/qq13-apis.html > > So I find any suggesting that docs are not necessary (or valueing merging > > higher) to be eventually shooting ourselves in the foot. > > Personally I think it would be very nice if source files had just a few > lines of comment telling what they are about, from a very high perspective. > Every time I am searching for the implementation of some functionality in > the OOo/LO codebase, I find myself looking at source files with vaguely > correct sounding file names (and vaguely correctly sounding class names > inside), but no clue if it actually is what I am looking for. > > (Yeah, now that we have forked off the codebase, I of course should take up > the habit of adding such a few comment lines every time I find out what the > code in some particular source file does, especially in cases where one can > easily be mislead and spend hours looking at irrelevant code before > realizing...) I think that would be very helpful. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Tuesday 30 of November 2010, Thorsten Behrens wrote: > Hi Lubos, > > adding documentation for what you describe doesn't help much, if at > all. Core API is reasonably documented, see offapi/udkapi/sal/basegfx. What is core API then? Do things under e.g. libs-core count as well? If yes, then just have a look there. > The most egregious cases of hard-to-grasp methods surely live in the > applications cores, use lots of weird abbreviations - and to > document their behaviour succinctly would prolly require as much > prose as there's code already inside them (i.e. multiple pages). ??? Documentation is not about stating again what can be read from the code, it is about summarizing and explaining it. A short description of a function can save the time reading it all (especially if non-trivial) or help finding the needed function, a description of strange code saves the time of figuring out why and what it does exactly. Nobody is talking about "int foo = 10; // assign 10 to foo". And if a function really needed multiple page of comments to explain it, then still I'd rather have that then spend a day (or even more, if it really required that much description) figuring it out the moment I need to touch it. > For me, a cleanly designed class with one and only one task (and a > proper mission statement, maybe - superfluous even for something > like "String"), does not need much method documentation. > Suitably-chosen method names, especially if they don't take 10 > parameters, and only work for exactly 42 combinations of them, are > almost self-documenting. Too bad LibO seems to be huge enough to contain many of those that do not fit this. > I simply refuse the notion that adding superficial documentation to > crappy api gets us anywhere - which does not rule out storing > hard-won knowledge of class purpose, inner workings, important pre- > or postconditions at times - and if it makes sense, as method- or > class-level doxygen documentation. I have no idea why you think I'm talking about superficial documentation. Besides, it's a question of what is superficial. If you think String is trivial, what does str.GetTokenCount('.') give you if str is "a.a." ? I spent several minutes on this and would have rather appreciated a one-liner saying what the function actually does. > But to really fix the problem you mention, I'm afraid only > large-scale refactoring helps. Quite possibly. But a) that's a lot more work, b) for that, it still helps to know what the code is supposed to do in the first place. > And wrong documentation surely is worse than no documentation. No idea why you think I'm talking about this either. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Wednesday 01 of December 2010, Thorsten Behrens wrote: > see? That's what I meant, documentation for most of the higher-level > methods is either > > a) superficial > b) so much prose that you're better off debugging the code in the >first place > > (bFull has *a lot* of side effects, and no, I did not bother to > research all of them for the while) See? So nobody actually knows what the code really does, and everytime somebody changes something there, they possibly have a slightly different understanding of the purpose, resulting in even bigger mess. That's yet another advantage of documentation - even you yourself are reminded of what the code is supposed to do, and everybody touching such code is less likely to run into a different direction. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
>> Personally I think it would be very nice if source files had just a few >> lines of comment telling what they are about, from a very high perspective. >> Every time I am searching for the implementation of some functionality in >> the OOo/LO codebase, I find myself looking at source files with vaguely >> correct sounding file names (and vaguely correctly sounding class names >> inside), but no clue if it actually is what I am looking for. >> >> (Yeah, now that we have forked off the codebase, I of course should take up >> the habit of adding such a few comment lines every time I find out what the >> code in some particular source file does, especially in cases where one can >> easily be mislead and spend hours looking at irrelevant code before >> realizing...) > > I think that would be very helpful. Totally agree. Sometimes, after floundering around in the code for a while, I suddenly come across a comment, and things get *so much* clearer, and feel I find myself overcome with overwhelming gratitude towards the mysterious comment writing person. Even if it's short and cryptic. And in German. It helps that much. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Wed, 2010-12-01 at 01:22 -0700, Tor Lillqvist wrote: > I am sure it is not hard to find people who think the OOo code APIs > are elegant, consistent and intuitive. And they would be equally > right. Well, I've been working on OOo (and now LibO) code base roughly about 6 years now, and I haven't come across a single person who thinks OOo's code APIs are elegant, consistent and intuitive. And I myself is not one of them. :-P But I've already become immune to this hard-to-figure-out code base, so I stopped thinking that way years ago. I've even trained myself to ignore those German comments. When I see functions, I see chunks of code, not functions. ;-) Kohei -- Kohei Yoshida, LibreOffice hacker, Calc ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Wed, 2010-12-01 at 13:37 +0100, Lubos Lunak wrote: > On Wednesday 01 of December 2010, Thorsten Behrens wrote: > > see? That's what I meant, documentation for most of the higher-level > > methods is either > > > > a) superficial > > b) so much prose that you're better off debugging the code in the > >first place > > > > (bFull has *a lot* of side effects, and no, I did not bother to > > research all of them for the while) > > See? So nobody actually knows what the code really does, and everytime > somebody changes something there, they possibly have a slightly different > understanding of the purpose, resulting in even bigger mess. That's yet > another advantage of documentation - even you yourself are reminded of what > the code is supposed to do, and everybody touching such code is less likely > to run into a different direction. I'm with Lubos here, but I believe we are slightly talking past each other. What Thorsten is saying (have idutils, ctags etc to power your editor) is pretty much required to battle this code base efficiently. But that doesn't take away the fact that the code base is massively under-documented or have cryptic structure in some places. And I've seen many instances of what Lubos is saying (multiple people adding their own stuff to the same method to bloat it into a kitchen sink that takes so many conditional bool parameters) happening in many areas of the code base. I would like to have some sort of tool to analyze the cyclomatic complexity of our code base to see if there are any hot spots in terms of code complexity. Now, I also agree with Thorsten that not all methods need to be documented; some methods are quite easy to figure out from their method names, and others are just methods extracted from other methods as part of refactoring, whose purpose are still unknown (they were just identical blocks of code that could be extracted into a common function). In the latter case I sometimes don't even know the intent of such common block of code. The practice I follow roughly is that, name a method something descriptive but not to make it too long, and if there are any gotchas or intent that can not easily be deciphered from the method name or the content of the code, I try to put that into its comment in the header. I think the common idiom is that, the code tells you what the method does, and the comment tells you what the intention of the method is. So, I personally think it's a good practice to spell out the intention of a method in its comment if it's not obvious. But I don't think we need to comment a method just to re-iterate what it does. Examples This is IMO excessive commenting. /** * Translate the passed string into English. * * @param str original string * * @return English translation. */ string translateToEnglish(const string& str); This is IMO good commenting. /** * Store new'ed instance of English translation into internal foo * container. The container manages its life-cycle, so don't delete * stored string instances! * * @return true if the string instance is successfully stored, false * if the string was not stored. The caller must delete the * string instance when this method returns false. */ bool storeEnglishTranslation(const string* pStr); Having said all this, I'm a bit concerned that if we start having a code-comment campaign, and end up having methods with incorrect comments because only the original authors of the methods know what the intents of their methods were. So, I'm not sure if we should go that far, especially with the existing methods whose authors were not any of us. Kohei -- Kohei Yoshida, LibreOffice hacker, Calc ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Wed, 2010-12-01 at 14:25 -0500, Kohei Yoshida wrote: > But I've already become immune to this hard-to-figure-out code base, so > I stopped thinking that way years ago. I've even trained myself to > ignore those German comments. When I see functions, I see chunks of > code, not functions. ;-) The ability to quickly read, and work effectively on a vast, gnarly, un-documented code-base is an incredibly valuable one - it turns out most commercial code-bases are a total mess internally; LibreOffice is nothing special. Furthermore, the code is -always- correct, and comments bit-rot at an amazing rate. However; I'm fairly convinced that there is a happy medium somewhere closer to Lubos than Thorsten but in-between - I don't think that investing tons of time into blindly documenting the behaviour of every method is at all necessary; yet clearly having good, descriptive per-method (and often more usefully per-class) documentation is really important. IMHO we cannot and should not put a break on writing that just to improve merge-ability, so I applaud another of Miklos' great hacks :-) Finally - wrt. ultimate code quality - I tend to think that growing our test suite can also enable the re-factoring that can make our APIs both pretty and intuitive over time, by providing good, clean, and wide ranging tests that will catch inadvertent changes in side-effects as/when they are introduced. Regards, Michael. -- michael.me...@novell.com <><, Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Lubos Lunak wrote: > See? So nobody actually knows what the code really does, and everytime > somebody changes something there, they possibly have a slightly different > understanding of the purpose, resulting in even bigger mess. That's yet > another advantage of documentation - even you yourself are reminded of what > the code is supposed to do, and everybody touching such code is less likely > to run into a different direction. > Hi Lubos, ok, so let me elaborate my (maybe extreme) point a bit. Documentation is a crutch, a feeble attempt to help us mere humans to grok big code bases. Good code needs only few comments, because it speaks for itself - it may need some mission statements on module- or class level, mostly to get the big picture across. And that is beneficial on many levels, none the least because, otherwise the more documentation you have, the higher the likelihood it's out-of-sync with your code. Sadly, we don't have good code in most places. So what benefit does documentation yield? Module- and class-level documentation will still be a helpful bridge between broader concepts (like "what's an EditEngine good for", what's the purpose of a "SwFrmFmt"), and bite-sized pieces of code someone could reasonable look into in finite time. But from method-level documentation, I expect a detailed, succinct description on what the method does, including error conditions, invariants, pre- and postconditions. Over the top you say? Nope. Because you'll need to know anyway, to be able to correctly use the method. That's why I'm cautiously avoiding to write detailed documentation for a function like QuickFormatDoc(bool) - I'll surely miss something, and even if not, it'll be wrong the day someone adds another bugfix to the edit engine. You're absolutely right with your statements above, that method purpose is often fuzzy, even messy - but that's *already the case* - adding documentation does not make it go away! The thing is, after 15 years of maintenance, most code, especially high-level one, is the opposite of general-purpose - it's been tweaked to interoperate with exactly the classes & methods it's interoperating with today. You need to use it in new contexts? Be prepared to fix it then. Does documentation help you there? Yes, if it helps you to understand the broader context quicker. Does detailed method-level documentation help you? I don't think so, for application-level code. It has an obscenely bad cost-benefit-ratio - better go read or debug the code. But you'll want good unit tests, to make sure your modifications didn't break existing clients (a way of "documenting" code btw, that's never out-of-sync, and cheaply verifiable - err, well, ideally ;)) In conclusion, maybe we're somewhat apart conceptually, but when actually adding documentation, I guess there will be little differences. I'd especially like to have vcl/tools/svtools classes reasonably documented - it's a sweet spot between effort, and benefitting many people at the same time. And you may notice that, if you find doxygen-style documentation in some gui, gfx, or impress header files, it's from me. ;) Cheers, -- Thorsten pgpKe6JCLBFPr.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Beware - I am a newcomer - maybe my point of view is quite wrong due to lack of understanding. But then at least I will understand something better :- ) My first remark: stepping in seems to me to be very difficult despite the very friendly behaviour of people here. > un-documented code-base is an incredibly valuable one - it turns out > most commercial code-bases are a total mess internally; LibreOffice is > nothing special. Gnap... depend of which software you are speaking, then... For having (to) read a lot of code in a big software, I can say that this one (I mean this commercial one) at least is well documented and quite well written. And no, I do not work for them:- ) > IMHO we cannot and should not put a break on writing that > just to improve merge-ability, Why not drop merging from OOO after LibO 3.3? As newcomer, I have read x times "not here due to merge / not touch due to merge"... Then I am reluctant to touch some parts...At least, the burden should be on side of person merging OOo into LibO, not be of a concern for people contributing to LibO. > enable the re-factoring that can make our APIs both > pretty and intuitive over time, I think would very help any newcomer stepping in. Currently hunting warning, I do it deliberately in order to get used to the code and uses. But even there, sometimes I skip the point, not being able after following the function calling the function calling... and determining for sure that the change would have a cross effect. And I do not even tried yet to change a feature. Deliberate choice from me, clear, but yeah... Regards ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
> Why not drop merging from OOO after LibO 3.3? Because there is still a significant number of people working on OOo, and they are in many cases the best experts there are on the code they are working on, and they are doing significant and useful improvements? Because there is no licensing reason that would prevent us from being able to take their code? Note that I am not saying we want to merge *everything* that OOo does. In some parts of the code, we might have good reasons to go a separate way. --tml ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Wednesday 01 of December 2010, Thorsten Behrens wrote: > ok, so let me elaborate my (maybe extreme) point a bit. > > Documentation is a crutch, a feeble attempt to help us mere humans > to grok big code bases. Good code needs only few comments, because > it speaks for itself - it may need some mission statements on > module- or class level, mostly to get the big picture across. And > that is beneficial on many levels, none the least because, otherwise > the more documentation you have, the higher the likelihood it's > out-of-sync with your code. The first line looks true, but the rest is from a superhuman, because for us mere humans documentation does have a value, even if the code would be perfect. It's mostly because we happen to be mere humans and do not have the brain capacity to remember everything or the time to analyse everything in detail. Documentation (and that is not just comments, but also API docs for example) is, if nothing else, a time-saver. To have a real example, see the sub-thread following http://lists.freedesktop.org/archives/libreoffice/2010-December/003756.html . Three developers, two of which I think quite highly of (I don't know Mattias), in a row failed to use properly a primitive class and ended up using a hacked up solution, because the moron that created this class saved at most few minutes by being too lazy to add at least "use SwCursor::RestoreSavePos()", eventually leading to much more time wasted and suboptimal code. A frustration-saver too, now that I think of it. I certainly had one of those WTF moments when trying to figure the problem out. > Sadly, we don't have good code in most places. So what benefit does > documentation yield? Module- and class-level documentation will > still be a helpful bridge between broader concepts (like "what's an > EditEngine good for", what's the purpose of a "SwFrmFmt"), and > bite-sized pieces of code someone could reasonable look into in > finite time. Have you actually ever worked with well-documented codebase? I can assure you that I've spent much more time reading API documentation of KDE library code (and saving me ever more time figuring it out myself) than I have spent writing it for my KDE library code (which is a non-trivial amount). > But from method-level documentation, I expect a detailed, succinct > description on what the method does, including error conditions, > invariants, pre- and postconditions. Over the top you say? Nope. > Because you'll need to know anyway, to be able to correctly use the > method. I see. Yes, your point maybe is extreme, because to me it looks like you're saying that documentation should be perfect and nothing less should be accepted, so if it can't be perfect, it has to be none, which will keep people clueless, so they at least won't touch the code. > That's why I'm cautiously avoiding to write detailed documentation > for a function like QuickFormatDoc(bool) - I'll surely miss > something, and even if not, it'll be wrong the day someone adds > another bugfix to the edit engine. You're absolutely right with your > statements above, that method purpose is often fuzzy, even messy - > but that's *already the case* - adding documentation does not make > it go away! Sorry, but you really do not understand what I am actually talking about. I'm not asking for anything ridiculous like having a paragraph explaining every line, I'm saying that everything non-trivial should have at least some documentation (or that should be at least goal, I don't expect everybody to start documenting everything today and dropping all other work). It is no big deal to require this for new code and whenever somebody runs into a problem or just feels like writing it down. KDE libraries are required to have this for all public API and nobody has a problem with that, and, just as well, it would be pretty unusual for 3 developers to fail using a class with just a ctor and a dtor. As for documentation getting out of date, especially when it's API description or code comments, I generally consider those getting obsolete being because the developer causing them was a moron who either broke public API or just quickly hacked up something without thinking about it. And even if that happens, I disagree with obsolete docs being worse than no docs - even wrong docs can help if you have a problem, you can search the history or similar, while without docs you're simply on your own and that's it. You know, if, after figuring out all these things about QuickFormatDoc(bool) and, you at least wrote down "the bool argument probably means that but it has a lot of side-effects", you would very likely still have helped Sebastian a lot. Even that counts, nobody wants a load of prose from you. > In conclusion, maybe we're somewhat apart conceptually, but when > actually adding documentation, I guess there will be little > differences. I'd especially like to have vcl/tools/svtools classes
Re: [Libreoffice] Script to find undocumented classes
Hi Lubos, you wrote: > I'm not asking for anything ridiculous like having a paragraph > explaining every line, I'm saying that everything non-trivial > should have at least some documentation > Good, then we're mostly on the same page. Then we're both talking about mission statements, higher-level interrelations etc. > You know, if, after figuring out all these things about > QuickFormatDoc(bool) and, you at least wrote down "the bool > argument probably means that but it has a lot of > side-effects", you would very likely still have helped Sebastian a > lot. > Well, I guess that's the point of contention we have here. A comment like that is useless in my book. And I somehow wanted to preemptively counter the notion that, like a cargo-cult, putting documentation on LibO code will magically make it intuitively usable - just because KDE is intuitively programmable, and has documentation. But I guess this topic is now thoroughly discussed to death, I hope my point, and my preference, is clear. We'd all love to have more documentation inside LibO, but neither me, nor anybody else I believe, will start mandating certain ways of documenting. We welcome cleanups, fixes and features - and if that comes with helpful header documentation, extra brownie points. ;) So just in case, let's agree to disagree & keep the patches coming! :) Cheers, -- Thorsten pgpRI3Gn8CfOF.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Hi all, On 2010-12-02 at 21:59 +0100, Thorsten Behrens wrote: > So just in case, let's agree to disagree & keep the patches coming! > :) As a conclusion, what about to combine Miklos' check for the missing documentation with a commit hook, so that it does not allow you to commit _new_ files without (at least the high level) documentation? ;-) Miklos - please, would that be doable without big impact on the speed of 'git commit'? Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Jan Holesovsky wrote: > As a conclusion, what about to combine Miklos' check for the missing > documentation with a commit hook, so that it does not allow you to > commit _new_ files without (at least the high level) documentation? ;-) > Hi Kendy, I find it surprising me actually saying this, but - for the while, I think this would be crossing the line of solving a social problem by technical means. ;) Cheers, -- Thorsten pgpHUGF9J5UDV.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Fri, Dec 03, 2010 at 11:14:54AM +0100, Thorsten Behrens wrote: > I find it surprising me actually saying this, but - for the while, I > think this would be crossing the line of solving a social problem > by technical means. ;) Additionally I'm not aware of a method to tell doxygen to check just a part of a file. And issuing a warning about 'class Foo is not documented' just because someone touched class Bar in the same file is indeed incorrect. ;) pgpgPNVYYxFDF.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On 03/12/10 12:11, Miklos Vajna wrote: > On Fri, Dec 03, 2010 at 11:14:54AM +0100, Thorsten Behrens > wrote: >> I find it surprising me actually saying this, but - for the while, I >> think this would be crossing the line of solving a social problem >> by technical means. ;) > Additionally I'm not aware of a method to tell doxygen to check just a > part of a file. And issuing a warning about 'class Foo is not > documented' just because someone touched class Bar in the same file is > indeed incorrect. ;) > I keep seeing all this stuff about Doxygen :-) I'm a bit of a newbie at this :-) so might it not be a good idea, on the wiki under development, to put a very basic page about doxygen with pointers to more detailed stuff? That way, people like me have a handle to get started. I know I can google for it, but a very basic "in libreoffice we do this" guide in the wiki would be a big help. Cheers, Wol ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Hi Miklos, Thorsten, On 2010-12-03 at 13:11 +0100, Miklos Vajna wrote: > > I find it surprising me actually saying this, but - for the while, I > > think this would be crossing the line of solving a social problem > > by technical means. ;) > > Additionally I'm not aware of a method to tell doxygen to check just a > part of a file. And issuing a warning about 'class Foo is not > documented' just because someone touched class Bar in the same file is > indeed incorrect. ;) That's why I highlighted that this would be done only with _new_ files, ie. the files that have have been git add'ed, and did not exist in the repository before. And we can further limit that only to .hxx/.h. As to the crossing the line - the first time it won't let you commit, and you'll be angry, the second time it won't let you commit, and you'll just fix that, and the third time you'll comment just naturally, and won't even hit the check :-) This worked with the warnings about how to structure the git commit logs [do you ever hit the hook that the 2nd line in the commit log should be empty?], so I don't see why it shouldn't work in this case ;-) Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Hi Wol, On 2010-12-03 at 13:43 +, Wols Lists wrote: > >> I find it surprising me actually saying this, but - for the while, I > >> think this would be crossing the line of solving a social problem > >> by technical means. ;) > > Additionally I'm not aware of a method to tell doxygen to check just a > > part of a file. And issuing a warning about 'class Foo is not > > documented' just because someone touched class Bar in the same file is > > indeed incorrect. ;) > > > I keep seeing all this stuff about Doxygen :-) I'm a bit of a newbie at > this :-) so might it not be a good idea, on the wiki under development, > to put a very basic page about doxygen with pointers to more detailed stuff? The hook can directly point to a page how to do it right ;-) Basically, instead of doing class Foo { int ugh; }; you'd do: /** This class implements access to your low level Foo machinery. It is to be used by This and That. [and probably some description of the pitfalls, if any] */ class Foo { int ugh; }; and you'd be done, the hook would be happy. Of course, special bonus would be for /// File descriptor of the Foo machinery. int ugh; Or did we actually not reached even this level of agreement? ;-) Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Fri, Dec 03, 2010 at 03:04:55PM +0100, Jan Holesovsky wrote: > That's why I highlighted that this would be done only with _new_ files, > ie. the files that have have been git add'ed, and did not exist in the > repository before. And we can further limit that only to .hxx/.h. Ah, that makes a difference. > As to the crossing the line - the first time it won't let you commit, > and you'll be angry, the second time it won't let you commit, and you'll > just fix that, and the third time you'll comment just naturally, and > won't even hit the check :-) This worked with the warnings about how to > structure the git commit logs [do you ever hit the hook that the 2nd > line in the commit log should be empty?], so I don't see why it > shouldn't work in this case ;-) I just added support for checking given files only (relative to the current directory) instead of the old "check all files in the current directory and subdirectories" behaviour. Should I write the git hook part of it, or can you do it? The only nontrivial part is how to to reach the scratch folder from the bootstrap repo. :) pgpsUPSCKnsqS.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Hi Miklos, On 2010-12-03 at 16:19 +0100, Miklos Vajna wrote: > > That's why I highlighted that this would be done only with _new_ files, > > ie. the files that have have been git add'ed, and did not exist in the > > repository before. And we can further limit that only to .hxx/.h. > > Ah, that makes a difference. Good :-) > > As to the crossing the line - the first time it won't let you commit, > > and you'll be angry, the second time it won't let you commit, and you'll > > just fix that, and the third time you'll comment just naturally, and > > won't even hit the check :-) This worked with the warnings about how to > > structure the git commit logs [do you ever hit the hook that the 2nd > > line in the commit log should be empty?], so I don't see why it > > shouldn't work in this case ;-) > > I just added support for checking given files only (relative to the > current directory) instead of the old "check all files in the current > directory and subdirectories" behaviour. Should I write the git hook > part of it, or can you do it? If you can do, that would be great. > The only nontrivial part is how to to reach the scratch folder from the > bootstrap repo. :) I'd say, just move it to bootstrap/bin, and enable in the bootsrap/git-hooks only? Thank you a lot, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Fri, 2010-12-03 at 10:23 +0100, Jan Holesovsky wrote: > Hi all, > > On 2010-12-02 at 21:59 +0100, Thorsten Behrens wrote: > > > So just in case, let's agree to disagree & keep the patches coming! > > :) > > As a conclusion, what about to combine Miklos' check for the missing > documentation with a commit hook, so that it does not allow you to > commit _new_ files without (at least the high level) documentation? ;-) I'm actually NOT in favor of this. As much as I believe in providing good code documentation for new code, this is a bit too far. Kohei -- Kohei Yoshida, LibreOffice hacker, Calc ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Fri, 2010-12-03 at 11:08 -0500, Kohei Yoshida wrote: > On Fri, 2010-12-03 at 10:23 +0100, Jan Holesovsky wrote: > > Hi all, > > > > On 2010-12-02 at 21:59 +0100, Thorsten Behrens wrote: > > > > > So just in case, let's agree to disagree & keep the patches coming! > > > :) > > > > As a conclusion, what about to combine Miklos' check for the missing > > documentation with a commit hook, so that it does not allow you to > > commit _new_ files without (at least the high level) documentation? ;-) > > I'm actually NOT in favor of this. As much as I believe in providing > good code documentation for new code, this is a bit too far. My rationale: Many times when I work on feature branches, I commit stuff but intentionally not provide documentation because the role of the class/method/whatever may change during the course of the implementation. This requirement would break my workflow, and I wouldn't appreciate that. Encouraging good documentation is a must, but making it a requirement even for new files unconditionally is bad. Kohei -- Kohei Yoshida, LibreOffice hacker, Calc ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Fri, Dec 03, 2010 at 11:14:12AM -0500, Kohei Yoshida wrote: > My rationale: Many times when I work on feature branches, I commit stuff > but intentionally not provide documentation because the role of the > class/method/whatever may change during the course of the > implementation. This requirement would break my workflow, and I > wouldn't appreciate that. > > Encouraging good documentation is a must, but making it a requirement > even for new files unconditionally is bad. Hm, what about enforcing it only in master / libreoffice-* branches? (Just an idea.) pgpfhUAnKQeGx.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
On Fri, 2010-12-03 at 17:34 +0100, Miklos Vajna wrote: > On Fri, Dec 03, 2010 at 11:14:12AM -0500, Kohei Yoshida > wrote: > > My rationale: Many times when I work on feature branches, I commit stuff > > but intentionally not provide documentation because the role of the > > class/method/whatever may change during the course of the > > implementation. This requirement would break my workflow, and I > > wouldn't appreciate that. > > > > Encouraging good documentation is a must, but making it a requirement > > even for new files unconditionally is bad. > > Hm, what about enforcing it only in master / libreoffice-* branches? > (Just an idea.) That would be fine with me. I would still like to avoid making it a requirement, but keeping it master only is bearable. I still prefer not having that as a requirement though. Running the missing-doc script every now and then like we do with cppchecks and Caolan's callcatcher would be my preferred approach. Kohei -- Kohei Yoshida, LibreOffice hacker, Calc ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] Script to find undocumented classes
Jan Holesovsky wrote: > As to the crossing the line - the first time it won't let you commit, > and you'll be angry, the second time it won't let you commit, and you'll > just fix that, and the third time you'll comment just naturally, and > won't even hit the check :-) This worked with the warnings about how to > structure the git commit logs [do you ever hit the hook that the 2nd > line in the commit log should be empty?], so I don't see why it > shouldn't work in this case ;-) > Hi Kendy, I was actually serious with my comment. The "2nd line must be blank" is a triviality, and a no-brainer. Banning tabs in code was at times actually work, and I've seen commits where people circumvented the hook. Forcing people to do what they wouldn't do otherwise is not the best strategy to get the best results... Cheers, -- Thorsten pgpbryZupxi7s.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice