[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[pepeto]

Update of bug #13848 (project freeciv):

  Status: In Progress => Fixed  
 Open/Closed:Open => Closed 

___

Follow-up Comment #12:

> Now, http://freeciv.wikia.com/wiki/Codin... needs to be updated.

Done.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[pepeto]

Update of bug #13848 (project freeciv):

  Status:  Ready For Test => In Progress

___

Follow-up Comment #11:

Now, http://freeciv.wikia.com/wiki/Coding_Style needs to be updated.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[pepeto]

Update of bug #13848 (project freeciv):

  Status: In Progress => Ready For Test 

___

Additional Item Attachment:

File name: trunk_CodingStyle.diff Size:23 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Matthias Pfafferodt]

Follow-up Comment #8, bug #13848 (project freeciv):

missed one comment wrt assert()

(file #8047)
___

Additional Item Attachment:

File name: version2-20100211-trunk-update-doc-CodingStyle.patch Size:20 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Matthias Pfafferodt]

Follow-up Comment #7, bug #13848 (project freeciv):

update doc/CodingStyle

* based on patch by Madeline Book 
* including all comments since this patch:
  - no more than 2 empty lines
  - prefix 'fc_'
  - '_new' / '_destroy' to allocated / free pointer variables
  - '_init' / '_free' to (de)initialize static data
  - use macros defined in log.h; do not use assert() / die()
  - include the header corresponding to the current c source file last

for trunk (and S2_2?)

(file #8046)
___

Additional Item Attachment:

File name: 20100211-trunk-update-doc-CodingStyle.patch Size:20 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[pepeto]

Update of bug #13848 (project freeciv):

  Status:None => In Progress
 Assigned to:   mbook => pepeto 


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Re: [Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Bernd Jendrissek]

On Fri, Jul 3, 2009 at 2:00 AM, Madeline
Book wrote:
> I also prefer the 0 rule because if we realize that
> case labels are essentially the same as goto labels,

Yesterday I was genuinely tempted for a minute to write some map
guessing code as:

  switch (tile_get_known(ptile)) {
  case TILE_KNOWN_FOGGED:
if (guessed_terrain->index == ptile->terrain->index) {
  case TILE_KNOWN:
  if (is_resource_valid(something_or_other)) { ... }
}
break;
...

I am going to assume that taking full advantage of the realization is
Strictly Verboten!

___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Re: [Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Per Inge Mathisen]

While on the topic of asserts, I recommend using a macro similar to
the ASSERT_OR_RETURN that I wrote for Warzone2100 --
http://warzone2100.svn.sourceforge.net/viewvc/warzone2100/trunk/lib/framework/debug.h?revision=7821&view=markup

The reason for this is that 1) you avoid having to do such double
evaluations, and 2) you quickly get into the (addictive) habit of
writing defensive code that fails softly for non-debug builds.

Also a more descriptive ASSERT() macro with comments (see above link)
helps when debugging.

Feel free to copy & paste this code ;)

  - Per

___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Madeline Book]

Follow-up Comment #2, bug #13848 (project freeciv):

> I don't like "switch" and "case" at same indentation
> level. I would vote for half indent step (2/2 = 1)
> for "case"

I do not feel that using one space indentation for
just this one area is very consistent with the two
space indentation rule used for most of the other
formatting rules. I think we should stick to units
of two spaces whenever indenting (or outdenting).


Using the attached perl script in the root of the
source tree:

$ grep switch `find -name \*.c` -A 3 -sn | ./switchstats.pl

gives the following statistics for the indentation
of the case label relative to the preceeding switch
statement:

-6 = 1
-5 = 25
0 = 405
1 = 22
2 = 485
3 = 1
4 = 2
5 = 1
8 = 3

Most follow the new style rule (0 = 405) and the old
convention of having the case label indented and then
also the case body indented again (2 = 485).

(The weird stuff like -5 and 8 is due to tabs mixed
into the leading whitespace, macros, or (I assume)
typos. If you run the script it will show the context
for these outlier values.)

When making the rule for the coding style, I rejected
the second style (2 = 485) even though it has more
occurences because I feel it causes too much
indentation of most of the switch body relative to
the switch statement (most code would be 2 levels out
from the switch statement, in contrast to all other
code blocks).

I also prefer the 0 rule because if we realize that
case labels are essentially the same as goto labels,
then the rule is consistent with the goto label
outdenting rule (everything in the switch body is
indented by one level (2 spaces), and the goto
labels (case labels) are outdented back one level
making them align with the switch statement again).


Unfortunately since we do not have that many active
maintainers, if you are not convinced by my arguments
this issue would be at a standstill. In that case I
would be open to just not including any switch-case
indentation rule in the guide, thus implicitly
allowing any not completely insane convention used
by contributors (i.e. 0, your 1+1, or 2 would be ok).



あの人もある時に痛いほど凹んでしまった。

(file #6117)
___

Additional Item Attachment:

File name: switchstats.pl Size:0 KB


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Marko Lindqvist]

Follow-up Comment #6, bug #13848 (project freeciv):

I discourage use of die() in most cases. I don't think CodingStyle should
advertise it.

Note that die() kills even release builds (those with NDEBUG) when assert()
does not. Of course failing assert() sometimes indicates problem that causes
crash anyway.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Marko Lindqvist]

Follow-up Comment #5, bug #13848 (project freeciv):

Reason why I want "case" labels intendet is that having ending "}" as first
line starting at the same level as "switch" itself makes it easier to see at
one glance where current "switch" ends. My eyes (and brain) just work that
way :-)

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Madeline Book]

Follow-up Comment #3, bug #13848 (project freeciv):

> Could you add assert() rule that instead of
> "assert(FALSE)" assert with more verbose error
> message should be used when possible: [...]

There is the die() function which can be used
if the assert condition is not descriptive
enough. I'll add a note about it and also to
avoid using "assert(0)" and similar.



ちょっと！まだ死なないでくれよ。

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Madeline Book]

Follow-up Comment #4, bug #13848 (project freeciv):

Some "rules" I was just reminded of:

- No more than 2 empty lines between any sections
  in the code.

- #include the header corresponding to the current
  c source file after all other headers.



思い出してくれてありがとう。

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Marko Lindqvist]

Follow-up Comment #1, bug #13848 (project freeciv):

Looks good in most part.

- I don't like "switch" and "case" at same indentation level. I would vote
for half indent step (2/2 = 1) for "case"
- Could you add assert() rule that instead of "assert(FALSE)" assert with
more verbose error message should be used when possible:

switch(value) {
 case 1:
 case 2:
   do_something();
   break;
 case 3:
   assert(value != 3);
   break;
}

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13848] [Patch] More detailed coding style guidelines

[Madeline Book]

URL:
  

 Summary: [Patch] More detailed coding style guidelines
 Project: Freeciv
Submitted by: mbook
Submitted on: Thursday 07/02/2009 at 02:33
Category: docs
Severity: 4 - Important
Priority: 5 - Normal
  Status: None
 Assigned to: mbook
Originator Email: 
 Open/Closed: Open
 Discussion Lock: Any
 Release: 
Operating System: None

___

Details:

As promised I expanded the coding style guidelines
with more specific rules, examples, and general
suggestions. I also made the guide itself follow
its own formatting rules.

I would please ask that everyone read the new guide
and comment if something is not acceptable or should
be added.

When this patch is applied to S2_1 and trunk, I will
also update the wiki page at:
http://freeciv.wikia.com/wiki/Coding_Style



新しい法律が発表されます。



___

File Attachments:


---
Date: Thursday 07/02/2009 at 02:33  Name: more_style.diff  Size: 18kB   By:
mbook



___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev