ITKv4 StyleChangeProposal: Difference between revisions

From KitwarePublic
Jump to navigationJump to search
(Created page with ' # Importance of consistent style It is recognized that preferences in coding style varies widely from developer to developer. It is, however, important that the entire ITK tool…')
 
No edit summary
Line 1: Line 1:
# Importance of consistent style
# Importance of consistent style
It is recognized that preferences in coding style varies widely from developer to developer.  It is, however, important that the entire ITK toolkit have a consistent well defined style so that the entire toolkit appears to have been written by a single developer.
It is recognized that preferences in coding style varies widely from developer to developer.  It is, however, important that the entire ITK toolkit have a consistent well defined style so that the entire toolkit appears to have been written by a single developer.
# Guidelines for changing the style
# Guidelines for changing the style
## Should be similar to the existing style
## Should be similar to the existing style
## Changes from existing style should be driven to ease maintenance of the the consistent style
## Changes from existing style should be driven to ease maintenance of the the consistent style
## (Others)
## (Others)
# Current Status
# Current Status
## The current style document was written 7 years ago and has not been updated since then, and is a great reference document, but contains some ambiguity/inconsistencies
## The current style document was written 7 years ago and has not been updated since then, and is a great reference document, but contains some ambiguity/inconsistencies
Line 14: Line 11:
##  function definitions are not nested inside the class level, and KWStyle requires that the braces are at column zero of the function
##  function definitions are not nested inside the class level, and KWStyle requires that the braces are at column zero of the function
## KWStyle has been given too much authority in dictating the style rather than just checking it.  We need to recognize that code that does not pass KWStyle may still be compliant with the standard, AND more importantly that making code pass KWStyle is in conflict with the guidelines.
## KWStyle has been given too much authority in dictating the style rather than just checking it.  We need to recognize that code that does not pass KWStyle may still be compliant with the standard, AND more importantly that making code pass KWStyle is in conflict with the guidelines.
# Proposal
# Proposal
## Rules should be as simple to follow as possible, with as few conditionals as possible  (i.e. all functions should have braces start  at either column 0 or column 2 irregardless of if it is a declaration or a definition, or if it is nested inside a class)
## Rules should be as simple to follow as possible, with as few conditionals as possible  (i.e. all functions should have braces start  at either column 0 or column 2 irregardless of if it is a declaration or a definition, or if it is nested inside a class)
Line 20: Line 16:
## Rules should be at least partially compatible with a wide range of editors (vim, emacs, VS) autoformatting capabilities (This may require more extensive style changes to make this occur)
## Rules should be at least partially compatible with a wide range of editors (vim, emacs, VS) autoformatting capabilities (This may require more extensive style changes to make this occur)
## KWStyle should be updated to allow passing of the style.
## KWStyle should be updated to allow passing of the style.
#Experience
The attached uncrustify (http://uncrustify.sourceforge.net/) configuration file closely matches the KWStyle (except for the inconsistent function brace indentation). http://www.itk.org/Wiki/images/6/62/Uncrustify_itk.txt  [[File:Uncrustify_itk.txt]]
<pre>
#!/bin/bash
#### Loop to auto check uncrustify.
BASEDIR=$1
cd $BASEDIR
RESULT_FILE=test.logger
OWF=/Users/johnsonhj/src/brains2/iplFreeware/Insight//Utilities/KWStyle/ITKOverwrite.txt
RULES=/Users/johnsonhj/src/brains2/iplFreeware/Insight//Utilities/KWStyle/ITK.cvs.kws.xml
rm -rf ${RESULT_FILE}
cd ${BASEDIR}
#for i in $(find . -name "*.cxx" |fgrep -v orig; find . -name "*.h"|fgrep -v orig; find . -name "*.txx"|fgrep -v orig); do
for i in $(find . -name itkHexahedronCellTopology.cxx); do
  ORIG=${i}.orig.cxx
  if [ ! -f ${ORIG} ]; then
    cp ${i} ${ORIG}
  fi
  ~/src/KWStyle-build/bin/KWStyle -v -o ${OWF} -xml ${RULES} ${ORIG} >> /dev/null 2>&1
  ORIG_STATUS=$?
  ~/local/bin/uncrustify -c ~/Desktop/AutoFormat/uncrustify_itk.cfg -l CPP -f ${ORIG} -o ${i}.temp >> /dev/null 2>&1
  diff  ${ORIG}  ${i}.temp > /dev/null 2>&1
  if [ $? -eq 0 ]; then
    rm ${i}.temp
  else
    mv ${i}.temp ${i}
  fi
  ~/src/KWStyle-build/bin/KWStyle -v  -o ${OWF} -xml ${RULES} ${i} |tee ${i}.kwerrors
  CLEANED_STATUS=$?
  if [ ${CLEANED_STATUS} -ne 0 ]; then
    echo "${i}: ${ORIG_STATUS} --> ${CLEANED_STATUS}" |tee -a ${RESULT_FILE}
  fi
  echo "========"
done |tee ERRORS.logger
</pre>

Revision as of 16:59, 20 July 2010

  1. Importance of consistent style

It is recognized that preferences in coding style varies widely from developer to developer. It is, however, important that the entire ITK toolkit have a consistent well defined style so that the entire toolkit appears to have been written by a single developer.

  1. Guidelines for changing the style
    1. Should be similar to the existing style
    2. Changes from existing style should be driven to ease maintenance of the the consistent style
    3. (Others)
  2. Current Status
    1. The current style document was written 7 years ago and has not been updated since then, and is a great reference document, but contains some ambiguity/inconsistencies
    2. The de-facto enforcement is that the code passes the KWStyle validation tester, and it appears that in at least one case (indentation) the KWStyle program is taking advantage of ambiguities in the style document to have different rules for indenting functions based on namespace and
    3. function declarations are nested inside namespace and class levels, and are currently KWStyle requires that the braces have a 2 space indentation
    4. function definitions are not nested inside the class level, and KWStyle requires that the braces are at column zero of the function
    5. KWStyle has been given too much authority in dictating the style rather than just checking it. We need to recognize that code that does not pass KWStyle may still be compliant with the standard, AND more importantly that making code pass KWStyle is in conflict with the guidelines.
  3. Proposal
    1. Rules should be as simple to follow as possible, with as few conditionals as possible (i.e. all functions should have braces start at either column 0 or column 2 irregardless of if it is a declaration or a definition, or if it is nested inside a class)
    2. Rules should be compatible with automated reformatting utilities (uncrustify)
    3. Rules should be at least partially compatible with a wide range of editors (vim, emacs, VS) autoformatting capabilities (This may require more extensive style changes to make this occur)
    4. KWStyle should be updated to allow passing of the style.


  1. Experience

The attached uncrustify (http://uncrustify.sourceforge.net/) configuration file closely matches the KWStyle (except for the inconsistent function brace indentation). http://www.itk.org/Wiki/images/6/62/Uncrustify_itk.txt File:Uncrustify itk.txt

#!/bin/bash

#### Loop to auto check uncrustify.
BASEDIR=$1
cd $BASEDIR

RESULT_FILE=test.logger

OWF=/Users/johnsonhj/src/brains2/iplFreeware/Insight//Utilities/KWStyle/ITKOverwrite.txt
RULES=/Users/johnsonhj/src/brains2/iplFreeware/Insight//Utilities/KWStyle/ITK.cvs.kws.xml

rm -rf ${RESULT_FILE}
cd ${BASEDIR}
#for i in $(find . -name "*.cxx" |fgrep -v orig; find . -name "*.h"|fgrep -v orig; find . -name "*.txx"|fgrep -v orig); do
for i in $(find . -name itkHexahedronCellTopology.cxx); do
  ORIG=${i}.orig.cxx
  if [ ! -f ${ORIG} ]; then
    cp ${i} ${ORIG}
  fi
  ~/src/KWStyle-build/bin/KWStyle -v -o ${OWF} -xml ${RULES} ${ORIG} >> /dev/null 2>&1
  ORIG_STATUS=$?
  ~/local/bin/uncrustify -c ~/Desktop/AutoFormat/uncrustify_itk.cfg -l CPP -f ${ORIG} -o ${i}.temp >> /dev/null 2>&1
  diff  ${ORIG}  ${i}.temp > /dev/null 2>&1
  if [ $? -eq 0 ]; then
    rm ${i}.temp
  else
    mv ${i}.temp ${i}
  fi
  ~/src/KWStyle-build/bin/KWStyle -v  -o ${OWF} -xml ${RULES} ${i} |tee ${i}.kwerrors
  CLEANED_STATUS=$?
  if [ ${CLEANED_STATUS} -ne 0 ]; then
    echo "${i}: ${ORIG_STATUS} --> ${CLEANED_STATUS}" |tee -a ${RESULT_FILE}
  fi
  echo "========"
done |tee ERRORS.logger