2018-08-19 20:28 BST

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0002692OpenFOAMContributionpublic2018-05-06 23:31
Reporterwyldckat 
Assigned Tohenry 
PrioritylowSeveritytextReproducibilityalways
StatusresolvedResolutionfixed 
Product Versiondev 
Target VersionFixed in Versiondev 
Summary0002692: Scripting for avoiding mismatches between header files and their macro bounds
DescriptionIn report #2686 - https://bugs.openfoam.org/view.php?id=2686 - was pointed out that there were header files that didn't have matching macros in the '#ifndef/#define' wrapping. I had a script working a few minutes after that issue was closed.

Anyway, attached is the script 'headerFix.sh' which will automatically scan and fix all header files '*.H' in '$FOAM_APP' and '$FOAM_SRC'.

I am planning on adapting this code to the git hook script(s).


However, there are a few header files that leave me wondering if we shouldn't adopt namespace based macro names? Here are a few examples that this script revealed:

 - src/finiteVolume/cfdTools/general/SRF/SRFModel/rpm/rpm.H - This has 'SRFModelRpm_H' instead of 'rpm_H'.

 - src/lagrangian/distributionModels/fixedValue/fixedValue.H - This has 'distributionModelFixedValue_H' instead of 'fixedValue_H', although there is no conflict with any other files.

 - src/rigidBodyDynamics/joints/composite/compositeJoint.H - Has 'RBD_joints_composite_H' instead of 'compositeJoint_H'.

TagsNo tags attached.
Attached Files
  • ? file icon headerFix.sh (473 bytes) 2017-09-09 17:28
  • ? file icon pre-commit-hook (10,488 bytes) 2017-09-09 18:07 -
    #!/bin/bash
    #---------------------------------*- sh -*-------------------------------------
    # =========                 |
    # \\      /  F ield         | OpenFOAM: The Open Source CFD Toolbox
    #  \\    /   O peration     |
    #   \\  /    A nd           | Copyright (C) 2011-2017 OpenFOAM Foundation
    #    \\/     M anipulation  |
    #------------------------------------------------------------------------------
    # License
    #     This file is part of OpenFOAM.
    #
    #     OpenFOAM 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 3 of the License, or
    #     (at your option) any later version.
    #
    #     OpenFOAM is distributed in the hope that it will be useful, but WITHOUT
    #     ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
    #     FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
    #     for more details.
    #
    #     You should have received a copy of the GNU General Public License
    #     along with OpenFOAM.  If not, see <http://www.gnu.org/licenses/>.
    #
    # Script
    #     pre-commit-hook
    #
    # Description
    #     pre-commit hook for git.
    #     Copy or link this file as ".git/hooks/pre-commit"
    #
    #     Eg,
    #     (
    #         cd $WM_PROJECT_DIR/.git/hooks &&
    #         ln -sf ../../bin/tools/pre-commit-hook pre-commit
    #     )
    #
    #     Hook receives: empty
    #
    #     Checks for
    #     - illegal code, e.g. <TAB>
    #     - columns greater than 80 for *.[CH] files
    #
    # Note
    #     Using "git commit --no-verify" it is possible to override the hook.
    #
    #     By supplying arguments to the hook, it can also be used to manually
    #     test the specified files/directories for standards conformance.
    #
    #------------------------------------------------------------------------------
    
    hookName="pre-commit"
    headerSeparator="-----------------------------------"
    
    die()
    {
        echo "$hookName hook failure" 1>&2
        echo $headerSeparator 1>&2
        echo '' 1>&2
        echo "$@" 1>&2
        echo '' 1>&2
        exit 1
    }
    
    #-----------------------------------------------------------------------------
    # Check content that will be added by this commit.
    
    if git rev-parse --verify HEAD > /dev/null 2>&1
    then
        against=HEAD
    else
        # Initial commit: diff against an empty tree object
        against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
    fi
    
    # called manually with arguments for the files/directories to be tested?
    if [ "$#" -gt 0 ]
    then
        case "$1" in
        -h | -help)
            die "interactive usage: supply list of files/directories to check"
            ;;
        esac
    
        # obtain list of all specified files/directories
        fileList=$(git ls-files -- $@ 2>/dev/null)
    else
        # list of all files to be committed
        fileList=$(git diff-index --cached --name-only $against --)
    fi
    
    #
    # no files changed: can skip all the checks
    # this usage can correspond to a 'git commit --amend'
    #
    [ -n "$fileList" ] || exit 0
    
    
    unset badFiles
    # join list of files with this amount of space
    Indent="    "
    
    #
    # report bad files and die if there are any
    #
    dieOnBadFiles()
    {
        if [ -n "$badFiles" ]
        then
            echo "$hookName hook failure" 1>&2
            echo $headerSeparator 1>&2
            echo "$@" 1>&2
            echo '' 1>&2
            echo "File(s):" 1>&2
            echo "$badFiles" 1>&2
            echo ''  1>&2
            exit 1
        fi
    }
    
    
    #
    # qualify 'git grep' to check cached value or from a specific commit
    #
    gitScope()
    {
        if [ "$#" -gt 0 ]
        then
            echo "$1:"
        else
            echo "--cached -- "
        fi
    }
    
    
    #
    # check for bad strings, characters, etc
    #
    checkIllegalCode()
    {
        echo "$hookName: check bad strings/characters etc ..." 1>&2
    
        reBad="("$'\t'")"
        msgBad="<TAB>"
    
        scope=$(gitScope $@)
    
        badFiles=$(
        for f in $fileList
        do
            case "$f" in
            # exclude potential makefiles
            (*[Mm]akefile* | wmake/rules/* | *.f* | *.v[cf]proj | *.pdf | *.png | *.html | *.gif | *.css | *.gz)
            ;;
            (*)
                fileType=`file -b $f`
                if [ "$fileType" != "data" ]
                then
                    # parse line numbers from grep output:
                    #        <lineNr>:   contents
                    lines=$(git grep -E -hn -e "$reBad" $scope"$f" |
                        sed -e 's@:.*@@' |
                        tr '\n' ' '
                    )
                    [ -n "$lines" ] && echo "$Indent$f -- lines: $lines"
                fi
            ;;
            esac
        done
        )
    
        dieOnBadFiles "Remove/correct bad '$msgBad' references"
    }
    
    
    #
    # limit line length to 80-columns
    #
    checkLineLength()
    {
        echo "$hookName: check line lengths ..." 1>&2
    
        scope=$(gitScope $@)
    
        badFiles=$(
        for f in $fileList
        do
            # limit to *.[CH] files
            case "$f" in
            (*.[CH])
                # parse line numbers from grep output:
                #        <lineNr>:   contents
                lines=$(git grep -hn -e '^.\{81,\}' $scope"$f" |
                    sed -e 's@:.*@@' |
                    tr '\n' ' '
                )
                [ -n "$lines" ] && echo "$Indent$f -- lines: $lines"
            ;;
            esac
        done
        )
    
        dieOnBadFiles "Limit code to 80 columns before pushing"
    }
    
    
    #
    # limit line length to 80-columns, except C++ comment lines
    #
    checkLineLengthNonComments()
    {
        echo "$hookName: check line lengths ..." 1>&2
    
        scope=$(gitScope $@)
    
        badFiles=$(
        for f in $fileList
        do
            # limit to *.[CH] files
            case "$f" in
            (*.[CH])
                # parse line numbers from grep output:
                #        <lineNr>:   contents
                lines=$(git grep -hn -e '^.\{81,\}' \
                    --and --not -e '^ *//' \
                    $scope"$f" |
                    sed -e 's@:.*@@' |
                    tr '\n' ' '
                )
                [ -n "$lines" ] && echo "$Indent$f -- lines: $lines"
            ;;
            esac
        done
        )
    
        dieOnBadFiles "Limit code to 80 columns before pushing"
    }
    
    
    #
    # limit line length to 80-columns, except #directive lines
    #
    checkLineLengthNonDirective()
    {
        echo "$hookName: check line lengths ..." 1>&2
    
        scope=$(gitScope $@)
    
        badFiles=$(
        for f in $fileList
        do
            # limit to *.[CH] files
            case "$f" in
            (*.[CH])
                # parse line numbers from grep output:
                #        <lineNr>:   contents
                lines=$(git grep -hn -e '^.\{81,\}' \
                    --and --not -e '^ *#' \
                    $scope"$f" |
                    sed -e 's@:.*@@' |
                    tr '\n' ' '
                )
                [ -n "$lines" ] && echo "$Indent$f -- lines: $lines"
            ;;
            esac
        done
        )
    
        dieOnBadFiles "Limit code to 80 columns before pushing"
    }
    
    
    #
    # check for non-standard code patterns
    #
    checkNonStandardCodePatterns()
    {
        echo "$hookName: checking for non-standard code ..." 1>&2
    
        scope=$(gitScope $@)
    
        badFiles=$(
        for f in $fileList
        do
            # limit to *.[CH] files
            case "$f" in
            (*.[CH])
                # Directly report the incorrect markers
                git grep -n --color -e '> >' \
                    --or -w -e 'NULL' \
                    $scope"$f"
            ;;
            esac
        done
        )
    
        dieOnBadFiles "$(cat<<MESSAGE
    Please revise the files reported below for the following non-standard code:
    
      1. Spaced ending of multi-level template parameters are not allowed, such as:
    
          List<List<scalar> >
    
        which instead should be:
    
          List<List<scalar>>
    
      2. The use of the 'NULL' macro should be replaced by 'nullptr'
    
    $headerSeparator
    MESSAGE
        )"
    }
    
    
    #
    # check that header files respect the policy of file names matching the
    # #ifndef/#define bounds
    #
    checkHeaderIfndefNames()
    {
        echo "$hookName: check header files #ifndef/#define names ..." 1>&2
    
        badFiles=$(
        for f in $fileList
        do
            # limit to *.H files
            case "$f" in
            (*.H)
                fileName=$(basename $f)
                correctMangledName=$(basename $f | sed 's=\.=_=')
    
                if grep -q "#ifndef.*_H" $f
                then
                  if ! grep "#ifndef.*_H" $f | grep -q $correctMangledName
                  then
                    echo "$(grep -H "#ifndef" $f | sed 's=#=\n\t#=') --> $correctMangledName"
    
                    currentMangled=$(grep "#ifndef.*_H" $f | sed 's=#ifndef\s*==')
    
                    sed -i -e 's='$currentMangled'='$correctMangledName'=' $f
                  fi
                fi
            ;;
            esac
        done
        )
    
        dieOnBadFiles "Some header files were automatically updated to respect #ifndef naming convention; Please check these before pushing"
    }
    
    
    #
    # check that OpenFOAM Foundation copyright is current
    #
    checkCopyright()
    {
        year=$(date +%Y)
        echo "$hookName: check copyright ..." 1>&2
    
        badFiles=$(
        for f in $fileList
        do
            startYear=`grep -m 1 "Copyright.*OpenFOAM" $f | sed 's/[^0-9]*\([0-9]*\).*/\1/g'`
            endYear=`grep -m 1 "Copyright.*-.*OpenFOAM" $f | sed 's/[^-]*-\([0-9]*\).*/\1/g'`
            #echo "startYear=$startYear endYear=$endYear"
            if [ "$startYear" != "" ]
            then
                if [ "$endYear" != "" ]
                then
                    # Date is of type 2011-2012 OpenFOAM Foundation
                    if [ "$year" != "$endYear" ]
                    then
                        echo "Updated copyright for: $f" 1>&2
                        echo "$f"
                        sed -i -e "s/$startYear-$endYear OpenFOAM/$startYear-$year OpenFOAM/g" $f
                    fi
                else
                    # Date is of type 2011 OpenFOAM Foundation
                    if [ "$year" != "$startYear" ]
                    then
                        echo "$f"
                        echo "Updated copyright for: $f" 1>&2
                        sed -i -e "s/$startYear OpenFOAM/$startYear-$year OpenFOAM/g" $f
                    fi
                fi
            fi
        done
        )
    
        dieOnBadFiles "Some copyright dates were automatically updated; Please check these before pushing"
    }
    
    
    #------------------------------------------------------------------------------
    # Main code : do all checks
    #
    
    # builtin whitespace check to avoid trailing space, including CR-LF endings
    bad=$(git diff-index --cached --check $against --) || die "$bad"
    
    # check for illegal code, e.g. <TAB>, etc
    checkIllegalCode
    
    # ensure code conforms to 80 columns max
    checkLineLengthNonDirective
    
    # check for non-standard code patterns
    checkNonStandardCodePatterns
    
    # check if #ifndef/#define bounds are named correctly
    checkHeaderIfndefNames
    
    checkCopyright
    
    exit 0
    #------------------------------------------------------------------------------
    
    ? file icon pre-commit-hook (10,488 bytes) 2017-09-09 18:07 +
  • ? file icon headerFix.v3.sh (1,561 bytes) 2017-09-10 23:59

-Relationships
related to 0002686closedhenry #ifndef *_H names don't necessarily correspond to *.H filename 
related to 0002920resolvedhenry Update for the 'Class' entries in the folder "src/regionModels/surfaceFilmModels/submodels", which are incomplete at the moment 
+Relationships

-Notes

~0008735

wyldckat (updater)

I forgot to mention that it checks all "*.H" files, but only tries to change those lines that have "#ifndef.*_H".

~0008736

wyldckat (updater)

Attached the file 'pre-commit-hook', for replacing the one at 'bin/tools/pre-commit-hook'.
It now checks header files that have "#ifndef.*_H" if they match the file name and they edit the file if not, similarly to how it handles the Copyright dates.

~0008738

henry (manager)

> However, there are a few header files that leave me wondering if we shouldn't adopt namespace based macro names?

In principle this would be a good idea but would add quite a bit of clutter. Currently I add namespaces to the macro name only when the name is simple and may generate a clash at some point however if we have a script which can generate and maintain the macro names with namespaces we could consider it.

In the longer term this will not be necessary as C++ will eventually support modules and at that point we should update OpenFOAM to use them and avoid all this macro nonsense.

~0008743

wyldckat (updater)

Attached is the script 'headerFix.v3.sh', which is an experimental attempt to define the macro name bounds based on namespaces+class name...
But the scripting I've managed to do so far with this version hits a few limitations, since I was only aiming for the common "namespace::namespace::...::class" convention for each header file, but there are several header files that do not follow this convention.

This is because the 'Class' block in the description is not reliable enough, so I had to make this script rely on figuring out the namespaces+class on its own... and fix the 'Class' block along for the ride too.

A few examples where this script got confused:

  - "Foam::fv::fv::optionList" on 'src/finiteVolume/cfdTools/general/fvOptions/fvOptionList.H'

  - "Foam::Foam::fixedFluxPressureFvPatchScalarField" on 'src/finiteVolume/fields/fvPatchFields/derived/fixedFluxPressure/fixedFluxPressureFvPatchScalarField.H'

  - "Foam::flipLabelOp" instead of "Foam::flipOp" on 'src/OpenFOAM/primitives/ops/flipOp.H', because the script is assuming that the last 'class' entry on the file is the one that gives it its name.


And this script does not cover header files that only define new classes as implementations of specific template classes... which means that on these files, it would be necessary to fall back on the filename based macro definition.


Using something like Vera++ - https://bitbucket.org/verateam/vera/wiki/Home - would likely be better for proper code style verification/enforcement, which seems to also have a tokenization mechanism which could potentially fix the two issues uncovered during the creation of these scripts:

 1. 'Class' in the description isn't always properly defined.

 2. Figuring out the correct namespace+class path isn't straight forward.


Next weekend I can try to come up with a mix of what I've gotten so far:

 - A git hook that checks if the 'Class' entry is properly define or not and suggest that perhaps it should be checked and changed manually...

 - Have a script that checks the 'Class' entries in bulk and changes them automatically, leaving repairs to be done manually

 - Updated the first proposition to have the '#ifndef' macro bounds rely on the 'Class' entry, when it's available, so that the namespaces+class are used instead of the file name.

~0008744

henry (manager)

> This is because the 'Class' block in the description is not reliable enough

Right, but this should also be fixed at some point.

> - A git hook that checks if the 'Class' entry is properly define or not and suggest that perhaps it should be checked and changed manually...

I think this is a good idea and probably the easiest and most reliable first step.

I have been thinking about the '#ifndef' macro and formally it should relate to the file name rather than the class name although ensuring consistency between these two would be good. The problem is then with handling conflicts: should this be done by including directory names in the macro name or namespace names?

I have applied your initial header fix and and it looks fine, I will complete tests and push it tomorrow.

~0008760

henry (manager)

I have just pushed the corrections generated by your script and the updated commit hook. I think this approach is sufficient for now until C++ gets proper module support.

Thanks for effort Bruno.

~0008761

henry (manager)

Resolved by commit 018adc16c91bfb10fd2e2061e63f31eff2d2fe7b
+Notes

-Issue History
Date Modified Username Field Change
2017-09-09 17:28 wyldckat New Issue
2017-09-09 17:28 wyldckat Status new => assigned
2017-09-09 17:28 wyldckat Assigned To => henry
2017-09-09 17:28 wyldckat File Added: headerFix.sh
2017-09-09 17:28 wyldckat Relationship added related to 0002686
2017-09-09 17:30 wyldckat Note Added: 0008735
2017-09-09 18:07 wyldckat File Added: pre-commit-hook
2017-09-09 18:07 wyldckat Note Added: 0008736
2017-09-09 21:56 henry Note Added: 0008738
2017-09-10 23:59 wyldckat File Added: headerFix.v3.sh
2017-09-10 23:59 wyldckat Note Added: 0008743
2017-09-11 00:11 henry Note Added: 0008744
2017-09-12 13:43 henry Note Added: 0008760
2017-09-12 13:43 henry Status assigned => resolved
2017-09-12 13:43 henry Resolution open => fixed
2017-09-12 13:43 henry Fixed in Version => dev
2017-09-12 13:43 henry Note Added: 0008761
2018-05-06 23:31 wyldckat Relationship added related to 0002920
+Issue History