[petsc-dev] I hate this one

[Karl Rupp]

Hi Matt,

 >     Again, there are limits to everything, and this surpasses the useful
>     limit to this kind of specification. This is not personal
>     expression, this
>     is ease of reading.
>
>
> Also, judging by the ENORMOUS number of source code changes, "everyone"
> was not following the informal guidelines.

Yes, the changes are 'enormous' if you count the absolute number of 
lines. However, my overall impression while refactoring was that I 
touched ~1% of the total code base only. It's the huge size of the code 
base that makes the number of changes appear to be high.

Also, an estimated 50% of the violations stem from a copy&paste of older 
code or previous examples rather than intentionally ignoring the style 
guidelines. Thus, by bringing everything to a consistent state, a 
(large) number of such copy&paste-induced violations are no longer going 
to happen at all.

The PETSc coding style guide needs to define exactly one style. Your 
style preference obviously differs. My personal preference differs 
slightly, too. Maybe Jed and/or others would prefer another slight 
variation. Nevertheless,  allowing a bit more flexibility in one's own 
personal coding style by following one common coding style is to the 
benefit of the whole project. Visual consistency is an obvious one, but 
most importantly it aids in setting up other scripts for increased 
uniformity among the different PETSc modules.

You may argue that it does not matter whether we have 'for(...)' or 'for 
(...)'. While this is certainly right for a compiler, I would like to 
give you one example where it *does* make a difference:
  src/mat/interface/matrix.c:
   PetscErrorCode  MatSetValuesAdifor(Mat mat,PetscInt nl,void *v)
So, keeping consistency with the style guide, we would write
   for (...) {...}
   MatSetValuesAdifor(...) {...}
and there is the additional blank contributing to the difference between 
the function call and the for-loop. If, however, we have
   for(...) {...}
   MatSetValuesAdifor(...) {...}
any scripts additionally need to check for characters in front of 'for'. 
Fortunately, it is *currently* sufficient to just consider all 
characters other than a blank in front of 'for' to be a function call. 
However, one day we might find code such as
   do_something(...);for(...) {...};
or even
   cilk_for(...)
and all of a sudden we are unable to use any simple checker scripts any 
more. With a common PETSc style, the additional blank would still allow 
for far simpler checks and refactoring if required.

Just my 2 cents...

Best regards,
Karli