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