[GRASS-dev] Re: GRASS 7 development started

[Ivan Shmakov]

>>>>> Paul Kelly <paul-grass at stjohnspoint.co.uk> writes:

 >>> Today the development of GRASS 6.4 has been moved out into an own
 >>> development branch. SVN trunk is now dedicated to GRASS 7
 >>> development which includes major refactoring of the code and
 >>> improvements to data formats etc.

 >> Can we finalise the coding style, so that we can apply it?

 >> This should be done before we start making substantial changes. Once
 >> the code has been re-formatted, it will become much harder to
 >> compare "before" and "after" versions, as any such diff will be
 >> dominated by the formatting changes.

 >> My personal preference is:

 >> indent -bad -bap -bbb -bl -bli0 -bls -cli0 -cs -fc1 -hnl -i4 -l79 \
 >> -nbbo -nbc -nbfda -nbfde -ncdb -ncdw -nce -nfca -npcs -nprs \
 >> -npsl -nsc -nsob -saf -sai -saw -sbi0 -ss -ts8 -ut

[...]

 >> Others will presumably have different preferences; in which case
 >> please express them sooner rather than later.

 >> The precise style doesn't really matter[1]; what DOES matter is that
 >> we have one style for all of GRASS, and that everyone abides by
 >> it[2].

 >> [1] Except for -ts8, which isn't a stylistic choice but a file
 >> format issue. Using something other than -ts8 is irrational; the
 >> only other sane alternative is to just forbid tabs outright (-nut,
 >> --no-tabs).

 > Allowing no tabs at all wouldn't be all that bad IMHO either. FFmpeg
 > does it and it makes it very simple to see if patches submitted there
 > comply with the whitespace formatting conventions - if there are any
 > tabs at all, it's rejected. My main query is whether code that
 > contains mixed tabs and spaces is "illegal" - I suspect it should be
 > - and if all occurences of 8 consecutive space characters are
 > required to be converted to a tab?

 > I find sometimes after a lot of editing of a piece of code and
 > adjusting the indenting, that the occurence of spaces and tabs
 > becomes quite mixed (generally as a result of me not knowing how to
 > set indentation settings in my editor). In this case I think not
 > allowing tabs and just spaces makes it easier to fix things up again
 > manually.

	There's no need to fix TABs manually, as there're already the
	`expand' and `unexpand' tools.

http://www.gnu.org/software/coreutils/manual/html_node/expand-invocation.html
http://www.gnu.org/software/coreutils/manual/html_node/unexpand-invocation.html

[...]

 >>  -cs, --space-after-cast Put a space after a cast operator.

 > When looking at the reformatted code this one jumped out at me as not
 > being the usual way I would write it. I think in my head the idea is
 > a cast normally does not actually modify the meaning of data the way
 > another operator might, and thus putting it right up against the
 > value being cast (without a space) somehow conveys that
 > better. That's all.

	Speaking of personal preferences, I'd prefer no space after a
	cast, too.

 > Another strange thing I noticed in the reformatted code (I'm not sure
 > which option causes it), is that the following code snippet

 > ---------------------
 >                 if (totalfonts >= maxfonts)
 >                 {
 >                     maxfonts += 20;
 >                     fontcap = (struct GFONT_CAP *) G_realloc(fontcap,
 >                                          maxfonts * sizeof(struct GFONT_CAP));
 >                 }
 > ---------------------
 > became this:
 > ---------------------
 >                 if (totalfonts >= maxfonts)
 >                 {
 >                     maxfonts += 20;
 >                     fontcap = (struct GFONT_CAP *) G_realloc(fontcap,
 >                                                              maxfonts *
 >                                                              sizeof(struct
 >                                                                     GFONT_CAP));
 >                 }
 > ---------------------

 > In the original example, when writing the code, I had moved the start
 > of the continuation line back to avoid something ugly happening like
 > in the indented code. I think the original version is easier to read
 > though.

	I think that it doesn't look too good either way.  You may
	consider introducing a temporary variable at the beginning of
	the block, e. g.:

   {
       void *new
           = G_realloc (fontcap,
                        ((maxfonts += 20, maxfonts)
                         * sizeof (*fontcap)));
       fontcap = (struct GFONT_CAP *)new;
   }

	At least as of GCC, `sizeof' allows either type name or an
	instance as an argument, and I feel that this feature is
	available at least as of ISO C89.  If it's indeed so, I think
	it's advisable to use it in most of the new code, as it makes
	the code somewhat easier to read.

	Alternatively, a helper macro could be used, e. g.:

#define REALLOC_ARY(x, y) \
    (realloc ((x), (y) * sizeof (*(x))))

/* ... */

   {
       maxfonts += 20;
       fontcap = (struct GFONT_CAP *)REALLOC_ARY (fontcap, maxfonts);
   }

	or:

#define REALLOC_ARY(x, y, cast) \
    ((x) = (cast)realloc ((x), (y) * sizeof (*(x))))

/* ... */

   {
       maxfonts += 20;
       REALLOC_ARY (fontcap, maxfonts, struct GFONT_CAP *);
   }

	GNU C allows for an even better variant:

#define REALLOC_ARY(x, y) \
    (((x) = (typeof ((x)))realloc ((x), (y) * sizeof (*(x)))))

/* ... */

   {
       maxfonts += 20;
       REALLOC_ARY (fontcap, maxfonts);
   }

	But it's apparently non-portable.

 > And that's about it for my preferences.