Add mechanisms to reformat and check code style, and reformat whole codebase (#128) #919
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --convert-tabs | ||
| --lineend=linux | ||
| --indent=spaces=4 | ||
| --style=kr | ||
| --add-braces | ||
| --max-code-length=80 | ||
| --break-after-logical | ||
| --pad-header | ||
| --pad-oper | ||
| --unpad-paren | ||
| --suffix=none | ||
|
There was a problem hiding this comment. @rouault Where does theses options come from ? Are they copy-pasted from QGIS ? There was a problem hiding this comment. All those options come from #128 (comment) |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| #!/bin/bash | ||
| ########################################################################### | ||
| # astyle.sh | ||
| # --------------------- | ||
| # Date : August 2008 | ||
| # Copyright : (C) 2008 by Juergen E. Fischer | ||
| # Email : jef at norbit dot de | ||
| ########################################################################### | ||
| # # | ||
| # This program is free software; you can redistribute it and/or modify # | ||
| # it under the terms of the GNU General Public License as published by # | ||
| # the Free Software Foundation; either version 2 of the License, or # | ||
| # (at your option) any later version. # | ||
| # # | ||
| ########################################################################### | ||
|
|
||
| for ASTYLE in ${OPJSTYLE} $(dirname $0)/opjstyle $(dirname $0)/RelWithDebInfo/opjstyle | ||
| do | ||
| if type -p $ASTYLE >/dev/null; then | ||
| break | ||
| fi | ||
| ASTYLE= | ||
| done | ||
|
|
||
| if [ -z "$ASTYLE" ]; then | ||
| echo "opjstyle not found - please enable WITH_ASTYLE in cmake and build it" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if type -p tput >/dev/null; then | ||
| elcr="$ASTYLEPROGRESS$(tput el)$(tput cr)" | ||
| else | ||
| elcr="$ASTYLEPROGRESS \r" | ||
| fi | ||
|
|
||
| if ! type -p flip >/dev/null; then | ||
| if type -p dos2unix >/dev/null; then | ||
| flip() { | ||
| dos2unix -k $2 | ||
| } | ||
| else | ||
| echo "flip not found" >&2 | ||
| flip() { | ||
| : | ||
| } | ||
| fi | ||
| fi | ||
|
|
||
| if ! type -p autopep8 >/dev/null; then | ||
| echo "autopep8 not found" >&2 | ||
| autopep8() { | ||
| : | ||
| } | ||
| fi | ||
|
|
||
| ASTYLEOPTS=$(dirname $0)/astyle.options | ||
| if type -p cygpath >/dev/null; then | ||
| ASTYLEOPTS="$(cygpath -w $ASTYLEOPTS)" | ||
| fi | ||
|
|
||
| set -e | ||
|
|
||
| astyleit() { | ||
| $ASTYLE --options="$ASTYLEOPTS" "$1" | ||
| #modified=$1.unify_includes_modified | ||
| #cp "$1" "$modified" | ||
| #scripts/unify_includes.pl "$modified" | ||
| #scripts/doxygen_space.pl "$modified" | ||
| #diff "$1" "$modified" >/dev/null || mv "$modified" "$1" | ||
| #rm -f "$modified" | ||
| } | ||
|
|
||
| for f in "$@"; do | ||
| case "$f" in | ||
| thirdparty/*) | ||
| echo -ne "$f skipped $elcr" | ||
| continue | ||
| ;; | ||
|
|
||
| *.cpp|*.h|*.c|*.h|*.cxx|*.hxx|*.c++|*.h++|*.cc|*.hh|*.C|*.H|*.hpp) | ||
| if [ -x "$f" ]; then | ||
| chmod a-x "$f" | ||
| fi | ||
| cmd=astyleit | ||
| ;; | ||
|
|
||
| *.py) | ||
| #cmd="autopep8 --in-place --ignore=E111,E128,E201,E202,E203,E211,E221,E222,E225,E226,E227,E231,E241,E261,E265,E272,E302,E303,E501,E701" | ||
| echo -ne "Formatting $f $elcr" | ||
| cmd="autopep8 --in-place --ignore=E261,E265,E402,E501" | ||
| ;; | ||
|
|
||
|
|
||
| *) | ||
| echo -ne "$f skipped $elcr" | ||
| continue | ||
| ;; | ||
| esac | ||
|
|
||
| if ! [ -f "$f" ]; then | ||
| echo "$f not found" >&2 | ||
| continue | ||
| fi | ||
|
|
||
| if [[ -f $f && `head -c 3 $f` == $'\xef\xbb\xbf' ]]; then | ||
| mv $f $f.bom | ||
| tail -c +4 $f.bom > $f | ||
| echo "removed BOM from $f" | ||
| fi | ||
|
|
||
| modified=$f.flip_modified | ||
| cp "$f" "$modified" | ||
| flip -ub "$modified" | ||
| diff "$f" "$modified" >/dev/null || mv "$modified" "$f" | ||
| rm -f "$modified" | ||
| eval "$cmd '$f'" | ||
| done |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| #!/usr/bin/env bash | ||
| ########################################################################### | ||
| # prepare-commit.sh | ||
| # --------------------- | ||
| # Date : August 2008 | ||
| # Copyright : (C) 2008 by Juergen E. Fischer | ||
| # Email : jef at norbit dot de | ||
| ########################################################################### | ||
| # # | ||
| # This program is free software; you can redistribute it and/or modify # | ||
| # it under the terms of the GNU General Public License as published by # | ||
| # the Free Software Foundation; either version 2 of the License, or # | ||
| # (at your option) any later version. # | ||
| # # | ||
| ########################################################################### | ||
|
|
||
| TOPLEVEL=$(git rev-parse --show-toplevel) | ||
|
|
||
| PATH=$TOPLEVEL/scripts:$PATH | ||
|
|
||
| cd $TOPLEVEL | ||
|
|
||
| # GNU prefix command for mac os support (gsed, gsplit) | ||
| GP= | ||
| if [[ "$OSTYPE" =~ darwin* ]]; then | ||
| GP=g | ||
| fi | ||
|
|
||
| if ! type -p astyle.sh >/dev/null; then | ||
| echo astyle.sh not found | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! type -p colordiff >/dev/null; then | ||
| colordiff() | ||
| { | ||
| cat "$@" | ||
| } | ||
| fi | ||
|
|
||
| if [ "$1" = "-c" ]; then | ||
| echo "Cleaning..." | ||
| remove_temporary_files.sh | ||
| fi | ||
|
|
||
| set -e | ||
|
|
||
| # determine changed files | ||
| MODIFIED=$(git status --porcelain| ${GP}sed -ne "s/^ *[MA] *//p" | sort -u) | ||
| #MODIFIED=$(find src -name "*.c") | ||
|
|
||
| if [ -z "$MODIFIED" ]; then | ||
| echo nothing was modified | ||
| exit 0 | ||
| fi | ||
|
|
||
| # save original changes | ||
| REV=$(git log -n1 --pretty=%H) | ||
| git diff >sha-$REV.diff | ||
|
|
||
| ASTYLEDIFF=astyle.$REV.diff | ||
| >$ASTYLEDIFF | ||
|
|
||
| # reformat | ||
| i=0 | ||
| N=$(echo $MODIFIED | wc -w) | ||
| for f in $MODIFIED; do | ||
| (( i++ )) || true | ||
|
|
||
| case "$f" in | ||
| thirdparty/*) | ||
| echo $f skipped | ||
| continue | ||
| ;; | ||
|
|
||
| *.cpp|*.c|*.h|*.cxx|*.hxx|*.c++|*.h++|*.cc|*.hh|*.C|*.H|*.sip|*.py) | ||
| ;; | ||
|
|
||
| *) | ||
| continue | ||
| ;; | ||
| esac | ||
|
|
||
| m=$f.$REV.prepare | ||
|
|
||
| cp $f $m | ||
| ASTYLEPROGRESS=" [$i/$N]" astyle.sh $f | ||
| if diff -u $m $f >>$ASTYLEDIFF; then | ||
| # no difference found | ||
| rm $m | ||
| fi | ||
| done | ||
|
|
||
| if [ -s "$ASTYLEDIFF" ]; then | ||
| if tty -s; then | ||
| # review astyle changes | ||
| colordiff <$ASTYLEDIFF | less -r | ||
| else | ||
| echo "Files changed (see $ASTYLEDIFF)" | ||
| fi | ||
| exit 1 | ||
| else | ||
| rm $ASTYLEDIFF | ||
| fi | ||
|
|
||
|
|
||
| # If there are whitespace errors, print the offending file names and fail. | ||
| exec git diff-index --check --cached HEAD -- | ||
|
|
||
| exit 0 | ||
|
|
||
| # vim: set ts=8 noexpandtab : |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that required ? Changing the whole project from C to C++ (also in cmake) just for opjstyle seems a bit overkill. No other option ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found hard with Travis to make it pass on all configurations and I'm afraid I didn't find a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any implication of changing compiler from gcc to g++ ? Does this prevent to compile on some older platforms ?
@malaterre any advice on this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmake will require a c++ compiler to be detected, even if it doesn't use it (it is only used if WITH_ASTYLE is ON, which is not the default). There's perhaps a way to restrict c++ compiler detection only when WITH_ASTYLE is ON but I couldn't find it.
The C compiler (gcc) will still be used to compile the .c files.
Anyway we require CMake to be able to build openjpeg and CMake source code itself is C++, so the platform must have a working C++ compiler.
And at some point we might perhaps want to use C++ for openjpeg itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rouault you are describing the symptoms not the real issue :) The fact that cmake only works on arch with valid c++ compiler is outside the scope of this question. I also fail to understand why
WITH-ASTYLE=ONimply a valid c++ compiler in the PATH. I do understand that it should have zero impact on the build itself (a bit more dependencies though), but I am just curious...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malaterre Astyle source code is C++ and apparently the fact that a project is c, c++ or both is declared in cmake at the PROJECT() scope, so as soon as you have C++ code you must enable C++. Is it really an issue to require a C++ compiler available ? I don't think so honestly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally failed to follow the fact that now openjpeg is bootstraping a c++ project (astyle). In that case, yes there isn't much solution. As said above
, I was just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks for your feedback @malaterre .
@rouault I think we can proceed and merge now.