Some warnings fixes and a question about boost python.

[John Peterson]

Hi,

I have pushed a few patches fixing warnings on the clang compiler under OSX in case you would like to pull them:
1. This line of code cannot be reached.
2. Remove redundant if.
3. Avoid warnings about unhandled enumerations.
4. Avoid warning about use of logical '&&' with constant operand.
5. Remove extraneous parentheses to silence warnings.

Also, is boost/python.hpp now required to build netgen? An unguarded inclusion of boost/python.hpp was added in 7f1cb899e52, but when I tried to guard it with #ifdef NG_PYTHON something else failed later in the compilation.

This leads me to a bigger question overall: would it be possible to disable Python completely off when configuring netgen? My interest is to build only the nglib C++ interfaces (no GUI, Python, etc.) for inclusion with the [libmesh] (github.com/libmesh/libmesh) finite element library, but the current Python 3 requirement makes that rather difficult.

Finally, I have one other issue when I try to configure with --disable-gui on OSX. This produces an empty TOGL_WINDOWINGSYSTEM variable in libsrc/visualization/Makefile and ng/Makefile that causes an invalid compilation command with a naked -D. I can work around the issue by modifying the generated Makefiles to read TOGL_WINDOWINGSYSTEM = TOGL_X11 but it would be nice to have a proper fix.

Thanks,
John

[Joachim Schoeberl]

Hi John,

thanks for the warnings of clang! They are serious: if the compiler eliminates checks to the (by the standard undefined!) null-pointers, the code will crash. So far we rely on behavior of current gcc, icc and msvc. We fix this when time is available. Your proposed fixes will also lead to crashes.

Did you try also cmake ?
We are currently switching from automake to cmake, and we certainly won't update automake.

Just added the new url to the netgen - summary page.

Joachim

[John Peterson]

if the compiler eliminates checks to the (by the standard undefined!) null-pointers, the code will crash.

I think you have perhaps misunderstood what the fixes do: You are testing the address of a C++ reference against NULL. By definition,

int x =1;
int & r = x;
assert (&r);

is guaranteed to work. Therefore the check is at best useless, and at worst misleading for someone reading the code.

Did you try also cmake ?

Not yet, but I will try that now.

[John Peterson]

The cmake build system does seem to work better than the automake one.

Am I correct in assuming that TCL, TK, and TIX are only required for the GUI? Here's the commands I ran (I have $CXX and $CC set correctly in my environment):

mkdir build
cd build
cmake .. -DINSTALL_DIR=/opt/petejw/netgen/git
         -DCMAKE_CXX_COMPILER=$CXX
         -DCMAKE_C_COMPILER=$CC 
         -DCMAKE_LINKER=$CXX 
         -DUSE_NATIVE_ARCH=OFF 
         -DUSE_GUI=OFF 
         -DUSE_PYTHON=OFF
 make

This seems to work just fine, until it's time to link libnglib.dylib, at which point I get:

ld: library not found for -lstlvis

The full output of make VERBOSE=1 is:

Linking CXX shared library libnglib.dylib
cd /Users/petejw/software/netgen/netgen_git/build/nglib && /opt/moose/cmake/bin/cmake -E cmake_link_script CMakeFiles/nglib.dir/link.txt --verbose=1
/opt/moose/mpich/mpich-3.1.4/clang-opt/bin/mpicxx   -std=c++11 -undefined dynamic_lookup -fopenmp -O2 -g -DNDEBUG -dynamiclib -Wl,-headerpad_max_install_names   -o libnglib.dylib -install_name /Users/petejw/software/netgen/netgen_git/build/nglib/libnglib.dylib CMakeFiles/nglib.dir/nglib.cpp.o ../libsrc/meshing/libmesh.so -lstlvis ../libsrc/stlgeom/libstl.dylib -lgeom2dvis ../libsrc/interface/libinterface.dylib ../libsrc/geom2d/libgeom2d.so ../libsrc/csg/libcsg.so ../libsrc/stlgeom/libstl.dylib ../libsrc/visualization/libvisual.dylib -lcsgvis /usr/lib/libz.dylib ../libsrc/meshing/libmesh.so /usr/lib/libz.dylib 
ld: library not found for -lstlvis

I checked, and there does not appear to be a -lcsgvis or -lgeom2dvis either. Do you know why it is trying to link these in?

[John Peterson]

I think I figured out the issue and fixed it here, when -DUSE_GUI=OFF is set, I don't think you can link against -lstlvis, -lcsgvis or -lgeom2dvis.

This allows tetgen to build and install, but leads me to another question: the installation does not seem to include nglib.h, which I had been using to interface with the library, inspired by the code in nglib.cpp. Is this intended, or an oversight in the cmake build system? (The file was installed by the automake build system.)

[Joachim Schoeberl]

Johnny,

I understand the case well. It's like below. According to the standard, the return value of GetFunnyRef is undefined - but:

gcc-5.2 doesn't complain at all
clang (Apple's 7.0.0) warns in Check, but is happy with GetFunnyRef.

Now, you remove the check - and you will actually access memory at NULL - bang

Thanks anyway for pointing out this issue, it's certainly in the code for 15 years. replacing NULL by nullptr shall fix it

#include<iostream>
double & GetFunnyRef()
{
  return *(double*)NULL;
}

void Check(double & a)
{
  if (&a == NULL)
    std::cout << "NULL" << std::endl;
  else
    std::cout << a << std::endl;
}

int main ()
{
  Check (GetFunnyRef());
}

Joachim

[Joachim Schoeberl]

and to the build ...
we are currently moving files around the libs, and not all configurations are regularly tested.
As you figured it out, just not linking against the missing libs if gui is off is fine.

and to nglib.h:
short answer: missing in cmake
longer answer: I'm not so happy with this old fashend nglib.h C interface, it will be replaced sooner or later by something else. If you encapsulate the Netgen - interface in your package, you might work directly with the internal classes.

and to tetgen:
no idea about that

[John Peterson]

This allows netgen to build and install, but leads me to another question: the installation does not seem to include nglib.h, which I had been using to interface with the library, inspired by the code in nglib.cpp. Is this intended, or an oversight in the cmake build system?

Anyway, this is easily fixed by a small change to nglib/CMakeLists.txt.

[John Peterson]

and to tetgen:
no idea about that

Sorry about the typo!!

[Joachim Schoeberl]

thanks for showing how to format code in here
I fixed it in my post above - and I also delete the non-matching replies
Joachim