CFD Online Logo CFD Online URL
www.cfd-online.com
[Sponsors]
Home > Forums > OpenFOAM Programming & Development

A few code questions

Register Blogs Members List Search Today's Posts Mark Forums Read

Like Tree1Likes
  • 1 Post By alexeym

Reply
 
LinkBack Thread Tools Display Modes
Old   May 31, 2015, 14:51
Default A few code questions
  #1
Senior Member
 
Alexey Matveichev
Join Date: Aug 2011
Location: Nancy, France
Posts: 1,112
Rep Power: 19
alexeym will become famous soon enoughalexeym will become famous soon enough
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

Code:
        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

Code:
        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:

Code:
    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?
alexeym is offline   Reply With Quote

Old   May 31, 2015, 17:48
Default
  #2
Super Moderator
 
Bruno Santos
Join Date: Mar 2009
Location: Lisbon, Portugal
Posts: 8,312
Blog Entries: 34
Rep Power: 84
wyldckat is just really nicewyldckat is just really nicewyldckat is just really nicewyldckat is just really nice
Greetings Alexey,

I won't be able to answer all of your questions, but let me see what I can answer:
Quote:
Originally Posted by alexeym View Post
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.


Quote:
Originally Posted by alexeym View Post
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:
    Code:
    #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.

Quote:
Originally Posted by alexeym View Post
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?
Code:
        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.

Quote:
Originally Posted by alexeym View Post
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:
    Code:
    virtual bool write() const;
  • "IOOutputFilter" has this:
    Code:
    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:
Code:
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.

Quote:
Originally Posted by alexeym View Post
5. This question is just about author intentions in readKivaGrid.H file. There is a loop on line 426

Code:
        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

Code:
        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:
Code:
-    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:
    Code:
    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.



Quote:
Originally Posted by alexeym View Post
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:

Code:
    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
wyldckat is offline   Reply With Quote

Old   June 1, 2015, 02:49
Default
  #3
Senior Member
 
Alexey Matveichev
Join Date: Aug 2011
Location: Nancy, France
Posts: 1,112
Rep Power: 19
alexeym will become famous soon enoughalexeym will become famous soon enough
Hi Bruno,

Thanks for the answer.

Quote:
Originally Posted by wyldckat View Post
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.

Quote:
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):

Code:
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
Code:
template<class T, class BinaryOp>
void reduce
(
    T& /* Value */,
    const BinaryOp& /* bop */,
    const int /* tag */,
    const label /* comm */,
    label& /* request */
)
{
...
}
2. Delete parameter names
Code:
template<class T, class BinaryOp>
void reduce
(
    T&,
    const BinaryOp&,
    const int,
    const label,
    label&
)
{
...
}
3. Use a variant of Q_UNUSED macro [3]

Code:
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?

Quote:
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?
Code:
        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:

Code:
virtual const labelUList& directAddressing() const
{
    ...
    return *directAddrPtr_;
}
and later

Code:
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.

Quote:
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
alexeym is offline   Reply With Quote

Old   June 12, 2015, 19:16
Default
  #4
Super Moderator
 
Bruno Santos
Join Date: Mar 2009
Location: Lisbon, Portugal
Posts: 8,312
Blog Entries: 34
Rep Power: 84
wyldckat is just really nicewyldckat is just really nicewyldckat is just really nicewyldckat is just really nice
Hi Alexey,

Sorry for the late reply...

Quote:
Originally Posted by alexeym View Post
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.


Quote:
Originally Posted by alexeym View Post
1. Comment out parameter names
Code:
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.:
Code:
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;
}

Quote:
Originally Posted by alexeym View Post
Yes, you got the question right. Yet I am talking about these constructions:

Code:
virtual const labelUList& directAddressing() const
{
    ...
    return *directAddrPtr_;
}
and later

Code:
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
wyldckat is offline   Reply With Quote

Old   June 17, 2015, 17:15
Default
  #5
Senior Member
 
Alexey Matveichev
Join Date: Aug 2011
Location: Nancy, France
Posts: 1,112
Rep Power: 19
alexeym will become famous soon enoughalexeym will become famous soon enough
Dear Bruno,

Thanks for the answer.

Quote:
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.

Quote:
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.

Quote:
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.
wyldckat likes this.
alexeym is offline   Reply With Quote

Reply

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are On


Similar Threads
Thread Thread Starter Forum Replies Last Post
some questions on using unsteady code pangpang Phoenics 0 January 25, 2002 23:22
Design Integration with CFD? John C. Chien Main CFD Forum 19 May 17, 2001 15:56
What is the Better Way to Do CFD? John C. Chien Main CFD Forum 54 April 23, 2001 08:10
own Code vs. commercial code Bernhard Mueck Main CFD Forum 10 February 16, 2000 11:07
public CFD Code development Heinz Wilkening Main CFD Forum 38 March 5, 1999 12:44


All times are GMT -4. The time now is 20:09.