On Mon, Jan 21, 2013 at 10:06 AM, Karl Rupp <rupp at mcs.anl.gov> wrote: > 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?
Thanks, Karl. I appreciate your effort on this front.