Remove cxa demangle cmake check

Summary

Remove cxa demangle cmake check, replaced by simple __has_include preprocessor check

Details

If the header <cxxabi.h> is available, the macro HAVE_CXA_DEMANGLE is defined. Instead of have a cmake test, just test für this header in the code.

changed the description

added 1 commit

• f59f7561 - remove cxa demangle cmake check, replaced by simple __has_include preprocessor check

Compare with previous version

added cleanup label

Looks good. Not sure whether we should default define HAVE_CXA_DEMANGLE in config.h for one more Dune release. But I guess it'll be fine.

resolved all threads

If nobody objects, I'll merge this after the upcoming weekend.

resolved all threads

mentioned in issue #199 (closed)

mentioned in issue #234 (closed)

added 40 commits

• f59f7561...803bd22d - 39 commits from branch master
• e2fbd664 - remove cxa demangle cmake check, replaced by simple __has_include preprocessor check

Compare with previous version

enabled an automatic merge when the pipeline for e2fbd664 succeeds

aborted the automatic merge because source branch was updated

added 3 commits

• e2fbd664...c36b4c3d - 2 commits from branch master
• fb1d5dd7 - remove cxa demangle cmake check, replaced by simple __has_include preprocessor check

Compare with previous version

Rebased, as it could no longer be merged.

enabled an automatic merge when the pipeline for fb1d5dd7 succeeds

merged

mentioned in commit ff9237b5

It seems that this MR breaks things in dune-istl. I do not yet know why. The registrytest fails if this branch is merged. I will have a quick look. If I can fix it, I will prepare an bugfix-MR. Otherwise, I would revert this MR and look into this a bit longer.

OK, the problem is not this MR, but a bug in the dune-istl test.

@simon.praetorius Porting my code from 2.7 to 2.8 I found out that I was including he DuneCxaDemangle CMake module. This makes a hard error in my build. I can fix it very easily so is not a problem at all, but I just wonder if a warning message would be better for at least one release.

I think this file was never intended to be included directly. It is part of the cmake feature tests. Yu get this included if you include the default cmake files. Just leaving an empty unused file would not make much sense. It is also not a recent change, done a couple of month ago.

We could, as backport into dune-2.8, just add an empty cmake file with that name. Then your cmake error will vanish. In c++ you have to do something different. The HAVE_CXA_DEMANGLE is set for backwards compatibility when you include the file classname.hh. I grep'd all the repositories I could find for anyone using these macros or cmake files and could not find anyone. Sorry, that this error happened.

Just fix your wrong code. I wouldn't build bridges for broken code, it should be fixed right away.

Just fix your wrong code.

This file was distributed and installed on past released versions, yet, the user is blamed for making use of it (?) I am not sure what to make out of this comment... IMHO, CMake modules should be treated as C++ headers in this respect. BTW, a similar situation happened with DuneMPI.cmake but there we have a warning.

Sorry, that this error happened.

Not a problem at all :) as I said, my code is fixed. I just wanted to let you be aware of the issue.

Just fix your wrong code. I wouldn't build bridges for broken code, it should be fixed right away.

I also think this comment was a bit too harsh words. The code was not really "wrong". Maybe just used something that was just an internal file, but was not marked as this.

We do not yet have a way to denote in cmake code that something is an implementation details, like the Impl namespace. If someone uses functions from the Impl then this code could break in a future release. But in cmake we do not have this. Maybe we should more properly document which files are allowed to be included in downstream modules and which functions can be used. Not all files contain macros and functions. Those could be appended with an underscore, for example, to indicate a private/internal nature.

I just wanted to let you be aware of the issue.

Thank you for pointing this out. In the future we should be more careful with removing code also from cmake.

Just an idea: We have the file DuneMacros.cmake. This is, I think, the only file from the cmake folder that should be included in downstream modules. Maybe a few others more. We could add a subdirectory impl into the cmake/modules/ folder that contains all the other files that are included from the main file. Then it is more clear that this should not be included manually.

Sorry, I didn't want to affront anybody by my words.

The find module files are not meant to be included anywhere else. The interface is the config.h file, and for this we offer a transition period. Simon's suggest won't work, as cmake/modules is automatically included in the search path for find modules, while a sub-folder impl would not be included.

I am not sure, if this is even documented.

I agree, the FindXYZ modules should stay in the modules folder. But other cmake files, we only include internally, could be included directly, maybe with additional subdirectory path. This idea still needs to be tested and I'm also not sure whether this is a good idea. Currently, it is just an idea.

We could add a subdirectory impl into the cmake/modules/

I think this could be the right direction but I second @gruenich: One problem I see with this is that modules and functions are still used in the same way (e.g. include(DuneMPI) or add_dune_mpi_flags("args...")) because the subdirectory does not add any scope to the module. I think the ideal case would be something that shouts to the user "I am an implementation detail, use me at your own risk!" when used. Now, since namespaces are non-existent on CMake, I think it has to be a name convention:

• Modules: Impl prefix. e.g. include(ImplDuneCxaDemangle).
• Functions: impl_ prefix. e.g. impl_add_dune_mpi_flags("args...").

mentioned in merge request !987 (merged)