|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0002692||OpenFOAM||Contribution||public||2017-09-09 17:28||2018-05-06 23:31|
|Target Version||Fixed in Version||dev|
|Summary||0002692: Scripting for avoiding mismatches between header files and their macro bounds|
|Description||In 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'.
|Tags||No tags attached.|
|I forgot to mention that it checks all "*.H" files, but only tries to change those lines that have "#ifndef.*_H".|
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.
> 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.
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.
> 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.
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.
|Resolved by commit 018adc16c91bfb10fd2e2061e63f31eff2d2fe7b|
|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|