Education ClassRoom/Previous Logs/OpenOffice.org Coding Guidelines
[16:59] <thorsten> --- classroom in ~1 minute ---
[16:59] <thorsten> there's a little intro presentation - http://wiki.services.openoffice.org/w/images/7/72/C%2B%2B- codingstandards.pdf
[17:00] * fredus (n=fredus@138.219.77-86.rev.gaoland.net) has joined #education.openoffice.org
[17:00] <metrokid> thank you for the file
[17:00] <thorsten> ok, hi everybody,
[17:01] <metrokid> hi
[17:01] <ericb2> thorsten: hello. Thanks for accepting to manage this ClassRoom.
[17:01] <thorsten> I'd like to start with the ooo coding standards classroom now,
[17:01] <Guillaume[ecn]> Hello
[17:01] <ericb2> @ all :
[17:02] <ericb2> we use to start with ~20 to 30 minutes of presentation, then questions
[17:02] <ericb2> but things can be adapted, of course
[17:02] <ericb2> thanks for your attention, and let's start :-)
[17:02] <thorsten> thanks ericb2 for the invitation,
[17:03] <thorsten> let's start with a quick motivational presentation about why coding standards & what for ;)
[17:03] <thorsten> #slide 2:
[17:03] <thorsten> has the link to the c++ coding standards for OOo,
[17:03] <thorsten> which I'll mostly focus on
[17:04] <thorsten> there are coding guidelines for perl & java as well,
[17:04] <thorsten> but ~90% of OOo is C++, so... ;)
[17:04] <thorsten> everybody able to see the media?
[17:04] <Guillaume[ecn]> yep
[17:04] <thorsten> good :)
[17:04] <thorsten> slide #3
[17:04] <IZBot> no issue with number 3
[17:04] <IZBot> no issue with number 3
[17:05] <thorsten> haha
[17:05] <thorsten> so, you can read the content for yourself -
[17:05] <thorsten> there's a bunch of reasons to have coding standards,
[17:05] * metrokid reading Origins of Standards
[17:05] <thorsten> for me, one of the most important is to have a common vocabulary,
[17:06] <thorsten> and, especially for c++, to limit the variations, i.e.
[17:06] <thorsten> have a somewhat bounded problem space, when trying to express a certain kind of functionality
[17:06] <thorsten> like - use c-like, all-macros kind of coding vs. using boost -
[17:07] <thorsten> if that's mixed, people have an even harder time reading other's code
[17:07] <thorsten> a second thing is, and that's prolly best shown by this page:
[17:08] * mmu_man has quit ("Vision[0.9.7-Z-101305]: i've been blurred!")
[17:08] <thorsten> http://wiki.services.openoffice.org/wiki/Writer_Code_Conventions
[17:09] <thorsten> to give best-practice examples of how other people solved generic problems
[17:09] <thorsten> @ all: questions so far?
[17:09] <thorsten> cool.
[17:10] <thorsten> so, we based this on some standard literature, and experience we had in OOo,
[17:10] <thorsten> but, like, this did not fall from the skies :)
17:10] <thorsten> slide #4, btw
[17:10] <IZBot> no issue with number 4
[17:11] <thorsten> slide 5:
[17:11] <thorsten> you speak up when I'm too fast or too slow, ok?
[17:11] <Guillaume[ecn]> k
[17:11] <ssaboum> ok
[17:11] <thorsten> cool
[17:11] <thorsten> so,
[17:11] <metrokid> ok
[17:11] <thorsten> having rules is all nice & sweet, but
[17:12] <thorsten> if you don't actually use them, e.g. by reviewing code for them,
[17:12] <thorsten> there's mostly theoretical value in them :)
[17:12] <thorsten> thus, coding standards & code reviews go more or less hand in hand.
[17:12] <thorsten> first, you learn the rules by checking for them,
[17:13] <thorsten> second, when doing a review, you need something to check for, right?
[17:13] * aude86_2008 (n=aude86_2@138.219.77-86.rev.gaoland.net) has joined #education.openoffice.org
[17:13] <thorsten> slide 6:
[17:13] <thorsten> that's how a code review could work -
[17:14] <ssaboum> http://wiki.services.openoffice.org/w/images/7/72/C%2B%2B-codingstandards.pdf
[17:14] <thorsten> thx ssaboum
[17:14] <aude86_2008> thank you ssaboum
[17:14] <ssaboum> lol sorry (someone didn't hav it )
[17:14] <thorsten> we're on slide 6 - how a code review against the standards could work
[17:15] <thorsten> don't spend too much time on it, one hour has proven manageable,
[17:15] <thorsten> and take turns in reviewing.
[17:15] <thorsten> take the review as suggestion, not correction -
[17:16] <thorsten> if somebody spots something in your code, actually, you might be right -
[17:16] <thorsten> OTOH, if you spot something in other people's code, assume, that *he* might be right, not you,
[17:16] <thorsten> so:
[17:16] <thorsten> it helps to present reviews in a non-insulting way :)
[17:17] <metrokid> ^_^
[17:17] <thorsten> slide 7:
[17:17] <thorsten> we would love feedback on the rules,
[17:18] <thorsten> if you miss important stuff, or if you thing something is nowadays a no-brainer, let us know -
[17:18] <thorsten> or, change it right away, it's a wiki :)
[17:18] * frouvier (n=frouvier@43.215.194-77.rev.gaoland.net) has joined #education.openoffice.org
[17:18] <thorsten> any questions so far, about the motivation & goals?
[17:19] <thorsten> cool,
[17:19] <thorsten> then, I'd like to move over to a concrete example:
[17:19] <ssaboum> :-)
[17:19] <thorsten> could everybody please view this page:
[17:19] <thorsten> http://wiki.services.openoffice.org/wiki/Cpp_Coding_Standards/GEN
[17:20] <Guillaume[ecn]> done
[17:20] <thorsten> ok, these are 8 easily checkable rules,
[17:20] <thorsten> in fact, each one of those topic pages is suitable for a code review,
[17:21] <thorsten> i.e. don't review for everything (at first), go for one topic.
[17:21] <thorsten> please, everybody quickly skim over it,
[17:21] <thorsten> and in like 3 minutes, we'll try and review concrete code with that
[17:22] <thorsten> ok: code is here:
[17:22] <thorsten> http://svn.services.openoffice.org/ooo/cws/canvas06/svx/source/svdraw/svdobj.cxx
[17:22] <thorsten> and
[17:22] <thorsten> http://svn.services.openoffice.org/ooo/cws/canvas06/svx/inc/svx/svdobj.hxx
[17:23] <ssaboum> ouch ...
[17:23] <thorsten> we go for the class SdrObject ctor
[17:23] <thorsten> yeah, it's kinda nasty code :)
[17:24] <ericb2> comments in German :)
[17:24] <metrokid> Warum ?
[17:24] <thorsten> yep, the example is hand-picked for the effect
[17:24] <thorsten> metrokid: why what?
[17:24] <ssaboum> for the french people out there ctor is "contructeur"
[17:25] <metrokid> Kommentare in Deutsch
[17:25] <thorsten> metrokid asks why the comments are in German -
[17:25] <Guillaume[ecn]> thx for the translation ^^
[17:25] <thorsten> and the answer is, this is code from StarDivision time,
[17:25] <thorsten> when dev was exclusively German
[17:25] <ssaboum> nice...
[17:26] <ericb2> thorsten: is the order of the 8 rules http://wiki.services.openoffice.org/wiki/Cpp_Coding_Standards/GEN the suggested order, or is it a random order ?
[17:26] <thorsten> more or less random,
[17:26] <ericb2> thorsten: ok
[17:26] <thorsten> there is no real precendence
[17:26] <ericb2> thorsten: optimize later, maybe ? :)
[17:26] <thorsten> ok, everybody looking at SdrObject::SdrObject() now?
[17:27] <ssaboum> nopeµ....
[17:27] <fredus> nope
[17:27] <thorsten> let's check for "Initialize Everything Immediately (Init)" in that method,
[17:27] <thorsten> tell me when you're ready
[17:28] <ssaboum> can you give a line please ?
[17:28] * ChanServ gives channel operator status to sm|CPU
[17:28] <Guillaume[ecn]> 441
[17:28] <metrokid> yep, need it too
[17:28] <Guillaume[ecn]> i think...
[17:28] <thorsten> yep
[17:28] <ssaboum> thanks guillaume ...
[17:28] <fredus> thx
[17:28] * metrokid at 441
[17:29] <Guillaume[ecn]> np
[17:29] <thorsten> cool, so, anybody spotting any violations of the Init rule?
[17:29] <Guillaume[ecn]> mnLayerID(0),?
[17:29] <ssaboum> ok
[17:29] <ssaboum> done
[17:29] <Guillaume[ecn]> mpSvxShape(0)?
[17:30] <thorsten> well, _those_ are initialized,
[17:31] <thorsten> but what about e.g. aOutRect ?
[17:31] <thorsten> (that's in the header, line 444
[17:31] <ssaboum> humm
[17:32] <thorsten> well - that member is a class, and has a default ctor.
[17:32] <thorsten> but that's not very readable, and one has to know a lot of context,
[17:33] <thorsten> so, the recommendation is to explicitely initialize *all* class members in the class initializer list,
[17:33] <thorsten> which is this list starting in the cxx, line 441
[17:33] <thorsten> that *also* include all those bSomething vars, which are, in this case,
[17:34] <thorsten> initialized *later* in the ctor body, for no real reason.
[17:34] <thorsten> it all boils down to maintainability -
[17:34] <thorsten> if you have all members listed in the initializer list, you can simply count them,
[17:35] <thorsten> and ensure that everything is properly initialized,
[17:35] <thorsten> and ensure that everything is properly initialized,
[17:35] <thorsten> was that halfway understandable? ;)
[17:36] <ssaboum> yep i got it (almost :-) )
[17:36] <thorsten> cool,
[17:36] <metrokid> got, it
[17:36] <thorsten> so, checking for the "init" item, we'd suggested the code owner to slightly refactor this ctor,
[17:37] <thorsten> and move the bSomething initialization to the initializer list.
[17:37] <thorsten> next item to check:
[17:37] <thorsten> " Clear Origin of Local Data (LocalData)"
[17:38] <thorsten> metrokid: could you give an example for a naming scheme that distinguished local vs. non-local data?
[17:38] <metrokid> ....
[17:38] <ericb2> thorsten: Impl... ?
[17:38] <ericb2> for locale
[17:38] <thorsten> yeah, that's common for local functions,
[17:39] <thorsten> for vars, the usual scheme is
[17:39] <thorsten> for class member variable, prefix an "m_" or only an "m",
[17:39] <thorsten> so, for a string, have
[17:39] <thorsten> m_aString as a class member,
[17:39] <thorsten> and aString as a local var.
[17:39] <ssaboum> ahhhhhh (the ah you sound when you get something )
[17:40] <thorsten> similarly, e.g. "g_" for globals,
[17:40] <thorsten> and "i_", "io_", "o_" for parameters,
[17:40] <thorsten> these are recommendations,
[17:41] <thorsten> unfortunately, except for the "m"/"m_" thingit, most of the code does not (yet) follow that ;)
[17:41] <thorsten> so:
[17:41] <thorsten> looking at the SdrObject ctor again, what can we spot?
[17:41] <ssaboum> mbLineIsOutsideGeometry
[17:42] <ssaboum> two m.....Something
[17:42] <thorsten> yep
[17:42] <ssaboum> but i don't get
[17:42] <thorsten> that one is alright - m for member var, b for boolean,
[17:42] * froumi (n=fred@trinity.ac-grenoble.fr) has joined #education.openoffice.org
[17:42] * valeuf (n=valeuf@kimsufi.anandra.org) has joined #education.openoffice.org
[17:42] * irc.freenode.net gives channel operator status to froumi
[17:42] <ssaboum> where is the sal_False thing ?
[17:42] * metrokid have to think about naming scheme....
[17:42] <Guillaume[ecn]> ok
[17:42] <fredus> ok
[17:43] <thorsten> ssaboum: FALSE, sal_False, and false are all ~the same,
[17:43] <thorsten> only that false (the c++ keyword) is to be preferred ;)
[17:43] <ssaboum> i get it
[17:43] <thorsten> hokay,
[17:43] <thorsten> so,
[17:43] <thorsten> the ctor shows how *not* to do it:
[17:44] <thorsten> the class has a bunch of members, that *mix* the naming scheme:
[17:44] * Sweetsha2k (n=bjoern@c209236.adsl.hansenet.de) has joined #education.openoffice.org
[17:44] <thorsten> there are pSomething members,
[17:44] <thorsten> mbSomething members,
[17:44] * fardad_afk is now known as fardad
[17:44] <thorsten> and in a larger function body, it's totally unclear which var is from the class, and which is function-local,
[17:44] <thorsten> and in a larger function body, it's totally unclear which var is from the class, and which is function-local,
[17:45] * soneca (n=ricardo@201.86.13.59.adsl.gvt.net.br) has joined #education.openoffice.org
[17:45] <thorsten> so, ssaboum, what would you suggest to the class author?
[17:45] <ssaboum> to rethink is naming scheme ??
[17:45] <thorsten> yeah :)
[17:46] <thorsten> but, in which way? ;)
[17:46] <ssaboum> adopt one way or the other but to stick to m_ or m and not mb... for members
[17:46] <ssaboum> but why did he put p ?
[17:46] <thorsten> go for mpSomething, mbSomething, etc.
[17:46] <ssaboum> pSomething
[17:47] <ericb2> ssaboum: pointers ?
[17:47] <thorsten> with p meaning: ptr , b meaning: bool,
[17:47] <ssaboum> ok
[17:47] <thorsten> r meaning: reference, s String, a Anything else :=)
[17:47] <thorsten> very good, questions so far?
[17:48] * metrokid some objective-C code to refactor tonight
[17:48] <Sweetshark> (another reason to initialize everything in the initializer list is that vars should be initialized the order in which they are declared. in a large class (like SwDoc), its hard to find the right position to init a var, if half the class is inited in the initializer list and half in the ctor-body.
[17:48] <ssaboum> what is the meaning of n ?
[17:48] <thorsten> integer number
[17:49] <ssaboum> oki
[17:49] <thorsten> f: floating point number
[17:49] * ericb2 suggests to build binfilter and log the warnings. As exercice ;-)
[17:49] <thorsten> right Sweetshark
[17:49] <thorsten> then, the last thing I'd like to mention is:
[17:49] <thorsten> "Local Conventions (LocalConv)"
[17:49] <thorsten> because:
[17:50] <thorsten> when you start working on OOo code, you prolly wont change naming across the board.
[17:50] <thorsten> so, to at least keep things consistent, adapt to the local conventions.
[17:50] <thorsten> when you find a mess like in SdrObject,
[17:50] <thorsten> where things *are* already inconsistent,
[17:51] <thorsten> choose the variant that's more correct according to the coding standards
[17:51] <thorsten> if write new code, and want examples of how things should ideally be:
[17:51] <thorsten> look at this page:
[17:51] <ssaboum> what would be the best way for sdrobject ?
[17:51] <ssaboum> everything in the constructor body ?
[17:52] <thorsten> the mpSomething, mbSomething scheme
[17:52] <thorsten> nope, move all initialization up into initializer list
[17:52] <ssaboum> ok
[17:52] <thorsten> http://wiki.services.openoffice.org/wiki/Writer_Code_Conventions
[17:52] <thorsten> has a lot of details regarding naming,
[17:52] <thorsten> code organization,
[17:53] <thorsten> and boost (and other) helpers,
[17:53] <thorsten> which should nicely complement the (rather abstract) coding standards,
[17:53] <thorsten> apart from that: if you miss something, please do add it, it's a wiki.
[17:53] <thorsten> if you're unsure, add it anyway, ;)
[17:54] <thorsten> and maybe ask here,
[17:54] <Sweetshark> if you are really unsure add it to the discussion page ;)
[17:54] <thorsten> ericb2 Sweetshark erack /me and others can surely help,
[17:55] <thorsten> yeah :)
[17:55] <thorsten> so, to wrap things up -
[17:55] <thorsten> OOo has a reasonable set of coding rules,
[17:56] <thorsten> separated into topics, that should be easily checkable during a 1 hour review sessiom,
[17:56] <ericb2> thorsten: yes. For the one who don't know them, Sweetshark is Bjoern Michaelsen , erack is Eike Rathke both are from Sun, and are often on the #dev.openoffice.org channel
[17:56] * chacha_chaudhry has quit ("Ex-Chat")
[17:57] <thorsten> take those rules with a grain of salt, there will be occasions when you can go against them.
[17:57] <thorsten> if in doubt, go with the rules.
[17:57] <thorsten> if you want a review, ping us,
[17:57] <thorsten> if you add something (details, new rules) to the wiki, I'll love you :)
[17:58] <ssaboum> ....thinking lol
[17:58] <ericb2> thorsten: I like the idea of the review, but that's not that easy to ask
[17:58] <ericb2> thorsten: now, when we have to modify existing code
[17:58] <metrokid> @thorsten do you get some good examples of *perfect code*, illustrating tough things ?
[17:58] <ericb2> thorsten: how do ? e.g. in svx or sfx2 or ..
[17:58] <thorsten> there's no perfect code ;)
[17:59] <thorsten> basically, you can review everything.
[17:59] <ssaboum> especially in objective-C
[17:59] <ssaboum> lol
[17:59] <ssaboum> lol
[17:59] <thorsten> when you do changes to existing code, make sure at least the changes don't make it worse,
[17:59] * ericb2 can read objective-C
[17:59] <metrokid> .... there is not that much coding standards coming from apple
[18:00] <ericb2> metrokid: hmm, when you are used to objective-C , mainly Mac OS X code, you are used to Apple headers
[18:00] <thorsten> metrokid: I think the c++ rules are pretty generic, most of them should apply to obj-c as well,
[18:00] <thorsten> but would love to have obj-c specifics as well, if you have something,
[18:00] <thorsten> go ahead
[18:00] <metrokid> yep, i think so, then , i will apply some ASAP [18:00] * ericb2 too
[18:01] <metrokid> ho, no ....
[18:01] <metrokid> ^^
[18:01] <thorsten> Sweetshark: anything you wrote recently you can recommend here as "role model code"?
[18:01] <thorsten> ;)
[18:01] <metrokid> there is some extra rules for messaging system
[18:02] <Sweetshark> thorsten: nope
[18:02] * thorsten would otherwise point to slideshow, which has reasonably modern c++ code
[18:02] <thorsten> but that of course a plug ;)
[18:02] * aude86_2008 has quit (" HydraIRC -> http://www.hydrairc.com <- The alternative IRC client")
[18:03] <thorsten> cool. so, if there are no more questions:
[18:03] <thorsten> thanks a lot for listening, it was fun with you!
[18:03] <ssaboum> thank you very much
[18:04] <Guillaume[ecn]> thanks!
[18:04] <ericb2> thaI got questions :)
[18:04] <ssaboum> it will surely help
[18:04] <frouvier> thx
[18:04] <ericb2> thorsten: I got questions :)
[18:04] <fredus> thx
[18:04] <thorsten> ericb2: shoot!
[18:04] <Pierre-Jean[ecn]> thx, pretty hard for a noob C++ student but interesting :)
[18:04] <fardad> thankx :)
[18:04] <ericb2> thorsten: in OpenOffice.org, is ther a work in progress to review/factorize the code ?
[18:04] <thorsten> hm,
[18:04] <Sweetshark> (as I just recently joined OOo development, I picked up conventions on the way, but those early code contribution might contain too many violations. of cause, that wont happen with any new cws ;D
[18:04] <ericb2> thorsten: my copy paste detector tilted :)
[18:04] <metrokid> nice classroom, thx, it will help, not only for OpenOffice
[18:05] <thorsten> ericb2: not across the board,
[18:05] <thorsten> ericb2: but quite a few people do it little-by-little,
[18:05] <ericb2> thorsten: ok. there is a lot of duplicated code, and I think most of the helpers are .. at lest dupe
[18:05] <ericb2> s/lest/least/
[18:05] <thorsten> e.g. the filter rewrites,
[18:05] <thorsten> yeah
[18:05] <ericb2> @all : the log of the ClassRoom is ... one minute
[18:06] <thorsten> that's right, but if everybody fights this just a little bit, things will improve ;)
[18:06] <thorsten> ericb2: so, the main reworks I know of are:
[18:06] <ericb2> thorsten: sure :-)
[18:06] <thorsten> slideshow
[18:06] <thorsten> drawing layer
[18:06] <thorsten> oox -
[18:06] <thorsten> and follow-up,
[18:06] <ericb2> thorsten: vcl ? ( /me hides )
[18:06] <thorsten> heh ;)
[18:06] <thorsten> the writer filter rework,
[18:07] <ericb2> @ all : the log of the ClassRoom , soon complete, is available there : http://wiki.services.openoffice.org/wiki/Education_ClassRoom/Previous_Logs/OpenOffice.org_Coding_Guidelines
[18:07] <thorsten> and some writer internal refactoring, Sweetshark might know more,
[18:07] <thorsten> IIRC ama is doing redlining/undo rework
[18:07] <Sweetshark> thorsten: Im currently doing lots of refactoring on marks/bookmarks in writer
[18:07] <thorsten> that rocks!
[18:08] <thorsten> previously, there was a replacement (kind of) for sfx2, which is in framework module,
[18:09] <thorsten> that makes e.g. chart2 completely independent from sfx2,
[18:09] <thorsten> that makes e.g. chart2 completely independent from sfx2,
[18:09] <thorsten> and probably a lot more I forgot -
[18:09] <thorsten> still, it's far from perfect ;)
[18:09] <thorsten> still, it's far from perfect ;)
[18:10] <ssaboum> i've got a question
[18:10] <thorsten> shoot
[18:10] <ssaboum> what is actually in place right now for the open office code
[18:10] <ssaboum> CVS ?
[18:10] <thorsten> subversion
[18:10] <ssaboum> with trac ?
[18:10] <thorsten> for the dev branch
[18:10] <thorsten> nah
[18:11] <ericb2> ssaboum: FYI : http://wiki.services.openoffice.org/wiki/Education_ClassRoom/Previous_Logs/OOo_svn_migration
[18:11] <Sweetshark> also stuff graduately mores away from the old implementations in the tools module as people are using their modern replacements ...
[18:11] <ssaboum> ok
[18:12] <thorsten> sorry folks, gotta run some errands, ttyl8er
[18:12] <ssaboum> ++
[18:12] <ssaboum> thanks again thorsten
[18:12] <ssaboum> bye
[18:12] <ericb2> thorsten: thank you very much for your time !!
[18:13] <ericb2> Sweetshark: thanks to you too :)
[18:13] <Sweetshark> ssaboum: currently, there is the lxr on go-oo.org for source browsing. But der will be a real nifty source browser on openoffice.org Real Soon Now(tm) on openoffice.org ...
[18:13] <Sweetshark> s/der/there/
[18:14] <Sweetshark> ericb2: np
[18:14] * frouvier (n=frouvier@43.215.194-77.rev.gaoland.net) has left #education.openoffice.org
[18:14] <ericb2> @all : all the logs of the previous ClassRooms are available too : http://wiki.services.openoffice.org/wiki/Education_ClassRoom/Previous_Logs
