[alexeym]

During ongoing attempt to build OpenFOAM with "-Wall -Wextra -std=c++11" flags without any warnings the following questions arose. Since they are not bugs and not a feature proposal yet, I have decided to discuss it here.

1. C++11 standard deprecates register variable modifier. Yet it quite frequently used in base library. Should it be kept in the code?

2. There are plenty of unused parameters in the code, which way of suppressing warning is preferred? There are several common methods: use UNUSED macro (which is basically UNUSED(x) (void)x), comment-out name of the parameter, just remove parameter name.

3. Though both g++ and clang produce code that can deal with NULL-pointer dereferencing into constant reference (directAddressing in FieldMapper.H is an example), stiil this behavior is undefined in standard. Should these parts kept as they are or should pointer-returning methods added to those cases?

4. In most cases implementation of child classes is OK, but from time to time developers desire strange things (functionObjects library is an example) like return type overload. I.e. functionObject class is a child of regIOobject, yet it is not happy with write method that returns bool, it would like to have write method returning nothing (i.e. void). Should such cases be prohibited (i.e. enforce correct method overload) or left as they are (allow method hiding)?

5. This question is just about author intentions in readKivaGrid.H file. There is a loop on line 426

        for
        (
            ;
            iterf != pFaces[WEDGE][0].end(), iterb != pFaces[WEDGE][1].end();
            ++iterf, ++iterb
        )
        {
	...
	}

comma operator discards left expression, i.e. result of the first comparison is just ignored. Should it be

        for
        (
            ;
            iterf != pFaces[WEDGE][0].end() && iterb != pFaces[WEDGE][1].end();
            ++iterf, ++iterb
        )
        {
	...
	}

?

6. This question is about author intention in surfaceBooleanFeatures.C. On line 357 there is calcFeaturePoints function. At the end of this function the following variables are defined:

    label concaveStart = 0;
    label mixedStart = 0;
    label nonFeatureStart = nFeaturePoints;


    labelListList featurePointNormals(nFeaturePoints);
    labelListList featurePointEdges(nFeaturePoints);
    labelList regionEdges;

and after these definitions function just ends. Is this just unfinished function or these definitions are there for particular reason?

[wyldckat]

Greetings Alexey,

I won't be able to answer all of your questions, but let me see what I can answer:

Originally Posted by alexeym

During ongoing attempt to build OpenFOAM with "-Wall -Wextra -std=c++11" flags without any warnings the following questions arose. Since they are not bugs and not a feature proposal yet, I have decided to discuss it here.

1. C++11 standard deprecates register variable modifier. Yet it quite frequently used in base library. Should it be kept in the code?

This one is beyond me I'm not familiar with the special modifiers that OpenFOAM relies on, such as the "register". And I'm not certain if OpenFOAM's source code is aiming to abide to the C++11 standard or to only use the new features that may be of interest.

Originally Posted by alexeym

2. There are plenty of unused parameters in the code, which way of suppressing warning is preferred? There are several common methods: use UNUSED macro (which is basically UNUSED(x) (void)x), comment-out name of the parameter, just remove parameter name.

If the warnings are in the applications, they could be suppressed, because these are usually because special ".H" files are common to several solvers and are included as such... but for example, not all compressible solvers handle trans/supersonic flow, hence the unused variables.
Preferably, the ".H" files that result in these warnings should be identified and separated into an hierarchical structure, e.g. (fake names used here):

• "transonicCreation.H" should include "standardCompressibleCreation.H" and append its specific settings, for example, the concept for "transonicCreation.H" would be:
  

  #include "standardCompressibleCreation.H"
  
  //transonic additions
  ...

• "standardCompressibleCreation.H" should contain the essential variable creation that is common to all compressible solvers and used by all.

On the other hand, if the warnings are in libraries, they should be fixed, in the sense that a full diagnosis should be done on a case to case basis.

Originally Posted by alexeym

3. Though both g++ and clang produce code that can deal with NULL-pointer dereferencing into constant reference (directAddressing in FieldMapper.H is an example), stiil this behavior is undefined in standard. Should these parts kept as they are or should pointer-returning methods added to those cases?

I'm not 100% certain I understood your exposition and question... but if I understood you correctly, you mean that perhaps methods such as this one should be added?

        virtual const labelListList * addressing() const

If this is the case... my guess is that the answer would be: no, such methods are probably not meant to be used in OpenFOAM, due to how the data is structured.

Originally Posted by alexeym

4. In most cases implementation of child classes is OK, but from time to time developers desire strange things (functionObjects library is an example) like return type overload. I.e. functionObject class is a child of regIOobject, yet it is not happy with write method that returns bool, it would like to have write method returning nothing (i.e. void). Should such cases be prohibited (i.e. enforce correct method overload) or left as they are (allow method hiding)?

I'm not sure I'm following correctly your idea, but the closest to "functionObject" being related to "regIOobject" that I could find is the class "IOOutputFilter" class. Where:

• "regIOobject" has this:
  

  virtual bool write() const;

• "IOOutputFilter" has this:
  

  virtual void write();

This one is tricky. I don't time to test this right now, but I'm guessing that the boolean method is not overloaded by the void method and that it should be possible to use the boolean one like this:

bool result = object.write();

But the fact is that the "read()" method is of type "bool" on both classes.

Nothing worst than ambiguity in coding... specially if one compiler chooses one method over the other on its own. So I guess that yes, the overloads should be stricter and if there is a reason for not being so strict, then the name of the methods should be different, to avoid collision or ambiguity.

Originally Posted by alexeym

5. This question is just about author intentions in readKivaGrid.H file. There is a loop on line 426

        for
        (
            ;
            iterf != pFaces[WEDGE][0].end(), iterb != pFaces[WEDGE][1].end();
            ++iterf, ++iterb
        )
        {
    ...
    }

comma operator discards left expression, i.e. result of the first comparison is just ignored. Should it be

        for
        (
            ;
            iterf != pFaces[WEDGE][0].end() && iterb != pFaces[WEDGE][1].end();
            ++iterf, ++iterb
        )
        {
    ...
    }

?

I've checked my combo repository: https://github.com/wyldckat/OpenFOAM-combo/ - and this file was overhauled in OpenFOAM 2.0.0. This particular for loop was one of the few or the only one that was not revised for the new formatting: https://github.com/wyldckat/OpenFOAM...readKivaGrid.H
For example:

-    for
-    (
-        SLList<face>::iterator iter = pFaces[CYLINDERHEAD][0].begin();
-        iter != pFaces[CYLINDERHEAD][0].end();
-        ++iter
-    )
+    forAllConstIter(SLList<face>, pFaces[CYLINDERHEAD][0], iter)

As for your proposed fix: yes, that seems to be the case, namely using the double ampersand, otherwise it risks a memory leak... although, an or makes more sense... and still...

Worst even, since it's an iterator, it might loop around on its own... so it might make sense to only stop for the second iterator...

Nah, something doesn't add up. This loop needs to be revised properly. This simply feels like accidentally bad coding went on in this loop. If the lists are not of the same size... OK, it's a wedge, so it's by definition always of the same size... or at least it should always be.

It seems that this code was left as-is for so long, because of the following premise:

• Both lists are of the same size, or at least they should be.
• Since they are of the same size, then one of the checks should be more than enough.
• Both were left in, as a reminder that this could should be revised if wedges ever become of different sizes.

I believe the correct fix for this should be:

1. Before the for loop, check:
   

   if(pFaces[WEDGE][0].size() == pFaces[WEDGE][1].size())

   If not, then it should issue an error stating that this is not supported.
2. In the for loop, checking for only one of the iterators should be enough.

As for the origin of this bug... it seems to exist at least since OpenFOAM 1.1.

Originally Posted by alexeym

6. This question is about author intention in surfaceBooleanFeatures.C. On line 357 there is calcFeaturePoints function. At the end of this function the following variables are defined:

    label concaveStart = 0;
    label mixedStart = 0;
    label nonFeatureStart = nFeaturePoints;


    labelListList featurePointNormals(nFeaturePoints);
    labelListList featurePointEdges(nFeaturePoints);
    labelList regionEdges;

and after these definitions function just ends. Is this just unfinished function or these definitions are there for particular reason?

This was added in OpenFOAM 2.3.0. It looks to me to be lost code from a debug session or an incomplete implementation.


Beyond this, I suggest that you deliver each patch individually (roughly 1 per question), whenever possible, otherwise it'll be a lot more complicated for the changes to be accepted. For discussion, it might make sense to report each fix in independent bug reports

Best regards,
Bruno

[alexeym]

Hi Bruno,

Thanks for the answer.

Originally Posted by wyldckat

This one is beyond me I'm not familiar with the special modifiers that OpenFOAM relies on, such as the "register". And I'm not certain if OpenFOAM's source code is aiming to abide to the C++11 standard or to only use the new features that may be of interest.

Well, as it is said in standard: "A register specifier is a hint to the implementation that the variable so declared will be heavily used." [1] (also a bit of explanation behind the decision in [2]), so it is just a hint for compiler. Also eventually C++11 will become default compilation mode and compilers start complain about register storage class.

If the warnings are in the applications, they could be suppressed, because these are usually because special ".H" files are common to several solvers and are included as such... but for example, not all compressible solvers handle trans/supersonic flow, hence the unused variables.
Preferably, the ".H" files that result in these warnings should be identified and separated into an hierarchical structure, e.g. (fake names used here):

Sorry, but you are mixing unused variable and unused parameter cases. I am talking about this case (let's take OpenFOAM/db/IOstreams/Pstreams/PstreamReduceOps.H as an example):

template<class T, class BinaryOp>
void reduce
(
    T& Value,
    const BinaryOp& bop,
    const int tag,
    const label comm,
    label& request
)
{
    notImplemented
    (
        "reduce(T&, const BinaryOp&, const int, const label, label&"
    );
}

all method parameters are ignored by implementation. I have several possible ways dealing with the situation:

1. Comment out parameter names

template<class T, class BinaryOp>
void reduce
(
    T& /* Value */,
    const BinaryOp& /* bop */,
    const int /* tag */,
    const label /* comm */,
    label& /* request */
)
{
...
}

2. Delete parameter names

template<class T, class BinaryOp>
void reduce
(
    T&,
    const BinaryOp&,
    const int,
    const label,
    label&
)
{
...
}

3. Use a variant of Q_UNUSED macro [3]

template<class T, class BinaryOp>
void reduce
(
    T& Value,
    const BinaryOp& bop,
    const int tag,
    const label comm,
    label& request
)
{
(void)Value;
(void)bop;
(void)tag;
(void)comm;
(void)request
...
}

So the question is: are there any community preferences?

I'm not 100% certain I understood your exposition and question... but if I understood you correctly, you mean that perhaps methods such as this one should be added?

        virtual const labelListList * addressing() const

If this is the case... my guess is that the answer would be: no, such methods are probably not meant to be used in OpenFOAM, due to how the data is structured.

Yes, you got the question right. Yet I am talking about these constructions:

virtual const labelUList& directAddressing() const
{
    ...
    return *directAddrPtr_;
}

and later

if (&map.directAddressing())
{
}

First it is dereferencing of the pointer. Both g++ and clang generate code that can cope with the case when directAddrPtr_ is null pointer. And they generate code that correctly converts constant reference into address. But still this behavior is not defined in standard. So this part of the code is compiler-dependent, and my proposal is to introduce directAddrPtr method that returns pointer, check if pointer is not null and only then convert pointer into constant reference.

I'm not sure I'm following correctly your idea, but the closest to "functionObject" being related to "regIOobject" that I could find is the class "IOOutputFilter" class.
...
Nothing worst than ambiguity in coding... specially if one compiler chooses one method over the other on its own. So I guess that yes, the overloads should be stricter and if there is a reason for not being so strict, then the name of the methods should be different, to avoid collision or ambiguity.

Yes, you got the idea right. I would say not just idea but you also pointed to correct origin of the mess

1. http://www.open-std.org/jtc1/sc22/wg...2011/n3242.pdf, 7.1.1
2. http://www.drdobbs.com/keywords-that...noth/184403859
3. http://code.qt.io/cgit/qt/qt.git/tre...31653a71#n1504

[wyldckat]

Hi Alexey,

Sorry for the late reply...

Originally Posted by alexeym

Well, as it is said in standard: "A register specifier is a hint to the implementation that the variable so declared will be heavily used." [1] (also a bit of explanation behind the decision in [2]), so it is just a hint for compiler. Also eventually C++11 will become default compilation mode and compilers start complain about register storage class.

Many thanks for the explanation!
I guess it depends on what is the oldest GCC version the OpenFOAM Foundation is willing to support for the latest OpenFOAM versions. They dropped support for GCC 4.4.x sometime ago in what seemed something mostly related to "we don't have time to test with so many GCC versions" than anything else.

Originally Posted by alexeym

1. Comment out parameter names

template<class T, class BinaryOp>
void reduce
(
    T& /* Value */,
    const BinaryOp& /* bop */,
    const int /* tag */,
    const label /* comm */,
    label& /* request */
)
{
...
}

I believe that this is the best option. I say this because OpenFOAM's source code is somewhat based on copy-paste-adapt strategies, which can make it easier to later diagnose what a derived class/method is meant to do with a particular variable.

The "Q_UNUSED" solution seems like an unnecessary amount of code and resulting binary code.

Although... I very vaguely remember seeing in OpenFOAM either solutions #1 or #2... but I can't remember where exactly and which ones
Wait... after a quick search... oh the irony... the names are omitted when issuing the FatalError message itself, but the method has the variables identified, e.g.:

Foam::label Foam::ptscotchDecomp::decompose
(
    const fileName& meshPath,
    const List<int>& initxadj,
    const List<int>& initadjncy,
    const scalarField& initcWeights,

    List<int>& finalDecomp
) const
{
    FatalErrorIn
    (
        "label ptscotchDecomp::decompose"
        "("
            "onst fileName&,"
            "const List<int>&, "
            "const List<int>&, "
            "const scalarField&, "
            "List<int>&"
        ")"
    )   << notImplementedMessage << exit(FatalError);

    return -1;
}

Originally Posted by alexeym

Yes, you got the question right. Yet I am talking about these constructions:

virtual const labelUList& directAddressing() const
{
    ...
    return *directAddrPtr_;
}

and later

if (&map.directAddressing())
{
}

First it is dereferencing of the pointer. Both g++ and clang generate code that can cope with the case when directAddrPtr_ is null pointer. And they generate code that correctly converts constant reference into address. But still this behavior is not defined in standard. So this part of the code is compiler-dependent, and my proposal is to introduce directAddrPtr method that returns pointer, check if pointer is not null and only then convert pointer into constant reference.

Oh my... I would prefer it would deliver an exception or a well documented crash, instead of allowing for dereferencing a null pointer.

Your proposition seems to make sense... problem is: how would that affect performance?

Best regards,
Bruno

[alexeym]

Dear Bruno,

Thanks for the answer.

I guess it depends on what is the oldest GCC version the OpenFOAM Foundation is willing to support for the latest OpenFOAM versions. They dropped support for GCC 4.4.x sometime ago in what seemed something mostly related to "we don't have time to test with so many GCC versions" than anything else.

In fact 4.4.x was dropped cause it does not support certain C++11 features I started using "-std=c++11" flag as clang emits warning since current code uses C++11 features.

I believe that this is the best option. I say this because OpenFOAM's source code is somewhat based on copy-paste-adapt strategies, which can make it easier to later diagnose what a derived class/method is meant to do with a particular variable.

So comments will it be.

Oh my... I would prefer it would deliver an exception or a well documented crash, instead of allowing for dereferencing a null pointer.

According to http://openfoam.org/mantisbt/view.php?id=1723, this issue is being addressed in OpenFOAM-dev. In fact the easies solution would be to use if(reinterpret_cast<const void*>(&iF) != NULL){...}, so complier does not remove this check due to optimizations.