-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 12/31/2014 06:51 AM, Timothy Pearson wrote: Hi Tim, code reformatting is definitely something we have to do. I meant to come back with some style ideas/rules last week but other things got into the way. Can you give me a few more days (say until Monday) to come back with my thought? In the mean time, I think it would be useful if you share a small piece of code showing the code formatting that you are thinking about. Since the code reformatting is a "one time op", we should make sure we are all happy with the code style we choose. Obviously this will require some compromise from each of us, since every one has its own preferences about style. Cheers Michele

If my vote still counts, I have only one request: please do not use 8 spaces for tabs. I prefer only 2 spaces, but 4 is acceptable. Darrell

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 2014/12/31 01:11 PM, Darrell wrote: I do not see any reason why your vote should not count. We may have different views on different things, but we are all here for the common goal of improving TDE. Cheers Michele

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA224 AStyle. In fact I was working on this in late 2012: https://git.trinitydesktop.org/cgit/scripts/tree/astyle Tim

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA224 Just one quick comment; I still need to carefully review your other suggestions and comment on them later: Detached braces are a big problem for me, severely reducing code legibility. Is this something you are flexible on or is code like this: void foo() { // Do stuff } hard for you to read? Thanks! Tim

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 <snip> In my programming carrier I have worked with different styles with different people, so if needed I can adapt to a different style and in general I am flexible. Whatever consistent style is better than "no style". The "attached brackets" vs "detached brackets" styles are two "schools of thoughts", so no wonder that if someone is used to one way he will find the other one harder to read, at least until he gets used to it. Having said all that, Slavek's comments described exactly my view on the bracket point: brackets at the end of the line are harder to find when scrolling along long blocks. I guess there is no easy solution, either we choose one way or the other. A possible way to mitigate the problem for the developers of the other school of thought could be the following (a little awkward but I haven't found anything better yet): 1) we create two (or more) style files and respective beautify files, for example one for attached brackets and one for detached brackets. The more options are in common between the two styles the better 2) we choose as "official style" the one most developers are happy with. 3) the developers of the other school can still use their style to edit code locally before committing using these steps: a) git pull b) beautify_to_the_style_I_like.sh c) edit, compile, test locally until happy d) beautify_to_TDE_official_style.sh e) commit It's a little bit uncomfortable, but at least its a possible way of working with one's own favorite code style. Note that point d) should become mandatory for all developers before any commit, to make sure the code style is maintain over time. Other ideas? Cheers Michele

On Thursday 01 of January 2015 13:31:04 Michele Calgaro wrote: For this I'll afraid that reformatting sometimes can cause unnecessary changes to the code just because of repeated reformatting. This could lead to unnecessarily hard to read commits. -- Slávek

On Thursday 01 of January 2015 21:43:08 Timothy Pearson wrote: Wow, indentation in twin is yet another - I would say, very confusing - because the brackets are at the same level of indentation as the block, so is lost completely. Awful. I am aware that the more popular now are brackets at the end of lines. And I, as well as Michele, earlier mentioned that we are flexibile and we have no problem adapt to it. In fact, it was enough for the pleasure to me knowing that along with Michele we have the same preference :) But what would prefer a smaller indentation in the switch cases - as featured Michele. Unfortunately I do not know whether astyle can be set to such a manner. See attachment. -- Slávek

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA224 <snip> My personal study of these adaptation phenomena has led me to the conclusion that, in most cases, the "adaption" is more an automated hiding of an inefficient and/or error-prone style from the level of the programmer's conciousness. Basically it shows up to an outside observer as a slowing down of the programmer's work without his or her knowledge, similar to clunky computer interfaces slowing down common tasks without the user's knowledge or inefficient factory stations draining employee productivity. It can even manifest as a subtle almost subconcious dislike of working on the code without the programmer ever really becoming aware of why. This is something we want to avoid if at all possible, hence this discussion. :-) Now of course there are cases where true adaption does take place, but there are limits on how far out the style can be from the programmer's natural expectations before this no longer holds. ;-) So, to be clear, this is what is desired? do_something(); switch(foo) { case bar: a=1; case baz: a=2; case asd: a=3; default: a=0; } do_something_else(); My question is this: Say I have a long case statement (dozens of cases with subsequent processing blocks, enough to run off both vertical edges of my screen) and it's "conveniently" located in the middle of a large source file with other case blocks and "normal code" above and below it. With the style above I will have problems finding the end of the case block, which is why I started indenting them like this: do_something(); switch(foo) { case bar: a=1; case baz: a=2; case asd: a=3; default: a=0; } do_something_else(); There are actually several instances of the pathological case described above in the TDE source tree right now, in the udev and network-manager interfaces among other locations, and I expect the number of these to grow as more DBUS-based system abstraction layers are forced on us (e.g. systemd, logind, and who knows what else down the line). Being able to easily say "yeah, I know what that case block does, get me to the next line of code after the case handler" by simply following the indentation proved quite valuable for me when writing the TDE hardware library. Rebuttals welcome. :-) I will definitely enforce the chosen style through an automated method; I'm not sure if I can practically hook into GIT for pre-commit as we have so many modules but I will be adding something to the server to make sure the style doesn't inadvertently "drift" over time. This is the current consensus as far as I can tell: 1.) Every conditional uses braces 2.) Hard tabs 3.) Conditional indentation like so: if (foo) { a=0; } else if (bar) { a=1; } else { a=2; } We still need to discuss how to handle large chains of conditionals, e.g. these pathological cases: https://git.trinitydesktop.org/cgit/tdelibs/tree/tdecore/tdehw/tdehardwared… Is the style shown there acceptable? Also on the docket are how to handle simple variable assignments; there are several ways I've seen: a=2; a = 2; a= 2; a =2; I'm fairly undecided on this. For consistency with larger statements "a = 1;" should probably be used but it seems such a waste of space compared with "a=1;". Thoughts? One other issue is the whitespace within conditional expressions. As you can see I'm fairly undecided on this as well, having used multiple styles already: https://git.trinitydesktop.org/cgit/tdelibs/tree/tdecore/tdehw/tdehardwared… https://git.trinitydesktop.org/cgit/tdelibs/tree/tdecore/tdehw/tdehardwared… Given a choice I slightly prefer the style shown on line 2518; my only strong preference here is that I don't want to see any whitespace between two matched parenthesis: Correct: execute_something() Wrong: execute_something( ) Thoughts are welcome here as well. Thanks! Tim

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA224 Ah, OK, that is a valid complaint. The case style shown above is fine and we'll go with that. My only remaining question then is should we also be forcing whitespace between each case block for legibility, something like this? do_something(); switch(foo) { case bar: a=1; break; case baz: { a=2; ...long case block... c=4; break; } case asd: a=3; break; default: a=0; } do_something_else(); I am curious as to why you find this more intuitive than having the logical operator before the comparison. Roughly translating to English the above style reads something like: if the box is black AND <pause> the box is not square AND <pause> the sphere is purple OR the sphere is NOT PRESENT THEN versus if the box is black <pause> AND the box is not square <pause> AND the sphere is purple <pause> OR the sphere is NOT PRESENT <pause> THEN You can see how in the latter style the relevant logical operator is stated first, then the expression, with all required information to understand the conditional contained on one line instead of spanning two lines. The latter is easier for me to grasp all of the conditions required in a large expression; the former requires a bit more effort. It isn't a big deal either way for me, and I'd like Slavek's input before deciding, but I am curious as to the thought processes behind the suggested style. :-) I'm going to lean towards forcing spaces for consistency. What is your opinion Slavek? OK, we'll go with that then. Good catch! Agreed. As the number of finalized rules is steadily growing I have started an Etherpad here to codify them and keep track of where we are at: http://trinity.etherpad.trinitydesktop.org/87 Tim

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA224 OK, agreed and added to official spec. Wouldn't the lack of a curly brace state the same thing, given our rules on curly braces? Point taken. However, I think we need to add a rule as well that states all conditionals must be enclosed in parenthesis; I have cleaned up so many compiler warnings and so much buggy code becase of lazy programmers in this respect that I don't want to see another unenclosed conditional. ;-) If this rule is enforced, your second example becomes: if ((a == 3) && ((b == 5) || (c == 4) || ((d == 10) && (e == 20)))) which is a bit easier to read, but overall this style is still harder to read than your first example when more than one condition is present on one line. Perhaps we should allow both styles and simply state that the style providing "best legibility" should be used? The number of long/complex ifs in the codebase are why I am insisting this be hammered out properly. :-) We haven't head from Slavek in a while so I guess we'll keep drawing up the specification and he can review it and comment when he has time. Yes! This is Hungarian notation for member variable scope, and should be strongly enforced. Added to spec for new code, though I don't think we can go back and fix this automatically. I prefer this: MyClass::MyClass(int i, char c, int *p) : m_i(i), m_c(c), m_p(p), m_d(d) { do_something(); } and where a derived class is being created: MyClass::MyClass(int i, char c, int *p) : TQObject(), m_i(i), m_c(c), m_p(p), m_d(d) { do_something(); } Is this acceptable? I have also added a question to the Etherpad regarding loop control statement formatting; if you could respond to that I'd appreciate it. Thanks! Tim

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA224 The problem with allowing these is that if I need to refactor the code to add in a single conditional I need to add parenthesis, which a.) is an additional source of error and b.) messes up the difference tracking making it hard for other developers to see what the functional change was. I think I'm going to override you on this one and force the parenthesis. ;-) But the base class(es) are fundamentally different than the other members; I like the visual distinction on first glance. Slavek, what is your opinion here? While that is more compact, it introduces a problem with maintainability. Say I need to add a new member variable, strongly associated with m_c. That changes the definition to: MyClass::MyClass(int i, char c, int *p) : m_i(i), m_c(c), m_cRel(c_r), m_p(p), m_d(d) { do_something(); } which a.) required quite a bit of editing (enough to make me think twice about adding the new member variable!) and b.) now shows two lines as completely changed in the difference output. The less-compact style makes it easy to add the variable, and shows at a glance what changed in the difference output. Tim

First of all: I was in earlier days very busy, so I did not manage to follow and respond to the debate on coding style. Now I will try gradually respond to individual discussed cases. On Friday 09 of January 2015 23:33:01 Timothy Pearson wrote: Here I would like to advocate for the previously mentioned principle braces even where they are not required. By this I mean that each 'case' consider as block => each case should to have brackets, including 'default': do_something(); switch(foo) { case bar: { a=1; break; } case baz: { a=2; ...long case block... c=4; break; } case asd: { a=3; break; } default: { a=0; } } do_something_else(); The advantage is that regardless of the size of the 'case' is easily traceable beginning of the block => editor can be used to trace paired brackets. With this would also not need white space between each 'case'. By the way, I wont give white space anyway. :) -- Slávek

On Sunday 11 of January 2015 22:20:50 Timothy Pearson wrote: For me also are certainly more readable operators at the end of the line, rather than at beginning. The only case where I'm used to using the operators at the beginning of the line is sql where I use words operators (and, or, ...) instead of symbols. By the way, in my usual style I'm used to indent brackets also in conditions, which from my perspective clarifies their nesting. For example: if (a == 3 && (b == 5 || c == 4 || (d == 10 && e == 20)) && (f == 15 || g == 25)) { } if (a == 3 && (b == 5 || c == 4 || (d == 10 && e == 20)) && (f == 15 || g == 25)) { } In this I agree with Michele. -- Slávek

On Friday 09 of January 2015 23:33:01 Timothy Pearson wrote: Yes, with spaces - in my view certainly readable. -- Slávek

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 01/17/2015 11:52 PM, Slávek Banko wrote: Good idea, makes the code easier to read. Cheers Michele

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 01/18/2015 02:24 AM, Slávek Banko wrote: The excessive brackets are a possible solution, but on the other hand are a little "too excessive" IMO. What about the following ideas: 1) use two or three spaces to separate for statement expressions, as in: for (i = 0; i < 1; i = i + 2) { or for (i = 0; i < 1; i = i + 2) { This should be relatively easy to read, maintain all conventions and does not require excessive/superfluous parenthesis. 2) use one line for each for expression as in: for (i = 0; i < 1; i = i + 2) { I also thought more about bracket position and came up with the following idea which is a mix of Tim's favorite style and mine/Slavek's favorite style. if (foo) { a = 0; b = 1; } else if (bar) { a = 1; b = 2; } else { a = 2; } switch(foo) { case bar: { a = 1; break; } case baz: { a = 2; ...long case block... c = 4; break; } case asd: { a = 3; break; } default: { a = 0; } } Basically, the only difference from Tim's style is that the opening bracket is detached and located in the following line, but the "space location" of the lines of code remains the same (no additional lines required only for the opening bracket). At the same time the opening and closing brackets are aligned, making easy to detect the beginning and ending of a block. What do you think? This is actually the first style I ever used many many many years ago :-) Cheers Michele

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA224 <snip> Sorry to be the odd one out here but that is rather difficult for me to read in addition to consuming a (IMHO) ridiculous amount of horizontal screen space. I understand the rationale, but now picture that statement in a sea of code--the eye will tend to scan right past it making determination of indent level difficult among other issues. Also, I have never seen this style in any other major project; please correct me if I am wrong. :-) I still need to go over this thread and respond to the other messages, but haven't had much time lately. Tim

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 01/21/2015 01:53 AM, Slávek Banko wrote: I also think the second is more readable than the first. Alternatively we can go for for (x = 1, y = 5; x < 10 && y > 0; x++, y--) although it is a waste of vertical space IMO. cheers Michele