Draft: Remove cpp definitions resolved in downstream modules

Description

Remove compiler dependent preprocessor definitions from the config files:

• DUNE_HAVE_CXX_EXPERIMENTAL_IS_DETECTED
• DUNE_HAVE_CXX_STD_IDENTITY
• DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA

This MR removes the CMake compiler/library tests for these definitions and provides Dune::Std::is_detected and Dune::Std::identity unconditionally. That is, independent of the compiler used in the downstream project, these two utilities will always be available. The DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA is completely removed since is not used in dune-common at all.

Motivation

The motivation of this change is to remove the dependency of config file between modules. Before this change, these cpp definitions where resolved in the downstream module. With this change, the config file is fully resolvable locally in dune-common and we can transition to a non dune specific build system in the spirit of #234 (closed).

See discussion here: #234 (comment 128723)

Note that if this is merged, we need to adapt <dune/grid/concepts.hh> to have another way to know if DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA is true (i.e., gcc>=9 or clang>=12)

added buildsystem label

Currently the description only covers why the MR is proposed. For completeness please add a description what it does.

done

Thanks a lot.

As far as I can see, one could add some more detail: This not just affects Dune::Std::is_detected and Dune::Std::identity but also all the other Dune::Std::*detected* utilities and Dune::Std::nonesuch. Also the description is a little bit misleading. These utilities have been available unconditionally before. The central change is that the dune 'fallback' implementation is now selected unconditionally.

Strictly speaking this is a breaking change. Although one could trigger this explicitly, users will hardly notice it in practice.

However, there's one exception: nonesuch is explicitly used as a signalling return type. Hence it's not unlikely that users explicitly test for it which makes it far more likely that the implicit assumption std::is_same<Dune:Std::nonesuch,std::experimental::nonesuch> was made somewhere. I don't see any way around this. But at least this potential source of problems deserves a changelog entry.

Thanks for the feedback. I did not know about this. How about we resolve explicitly these flags in the config file similar to what @simon.praetorius proposes for the DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA? I guess our compiler support implies standard library support for libstdc++ and libc++, so I can investigate on which versions were these types/typedefs added.

Regarding the nonesuch assumption. Why would you assume that nonesuch is explicitly the one from std::experimental? Maybe I miss a point here.

The problematic case would happen if someone uses Dune:Std::is_detected<....> and checks if the result is std::experimental::nonesuch. When you restrict yourself to newer compilers downstream, this was working so far. I cannot estimate how likely this is, but the problematic think is that the check would silently give the 'wrong' result after this change. This is probably unavoidable. But it's breaking change and a potentially nasty pitfall that at least deserves to be documented properly.

BTW: During the transition from our own shared_ptr to std::shared_ptr I remember several occasion where I have seen a mixture of Dune::shared_ptr with std::shared_ptr in downstream modules. This was safe, because the downstream module required a newer compiler and thus could rely on the fact, that Dune::shared_ptr is std::shared_ptr.

@carsten.graeser, is the explicit versioning of libstdc++ and libc++ fine for you? That approach would not affect people using these two standard libraries, but may potentially affect others...

changed the description

The motivation for having DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA in dune-common was (as far as I remember) that we wanted to not repeat the implementation of this check over and over again wherever we want to use concepts with lambdas. What about the following: instead of running a compiler check, we define this constant as a condition on other constants, e.g. the compiler versions

#ifndef DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA
#if defined(__clang__)
  #if __clang_major__ >= 15
    #define DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA
  #endif
#elif defined(__GNUC__) || defined(__GNUG__)
  #if __GNUC__ >= 9 || __GNUG__ >= 9
    #define DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA
  #endif
#endif
#endif

This could still be put into the config.h file. Maybe this could be refined a bit further.

Maybe this macro could also be put into the dune/common/concepts.hh file, the entry point for using concepts with dune. Then no extra definition in the config.h is needed.

This macro is not enough:

• lambda in unevaluated context is not precise enough (see comment !1261 (comment 133201)). We need to differentiate. Something works in clang-13 already other thing in clang-17 only.
• it is activated only if c++-20 flag is set (this can be checked)

Or we can just get rid of this requirement by not using those lambdas: dune-grid!726 (merged) ;)

this would even be better :-)

Now that dune-grid!726 (merged), shall we just remove this macro test?

BTW, when transitioning to C++20 we need to be careful to not introduce this dependency again until all supported compilers have implemented feature, e.g., staging/dune-typetree!124 (comment 133725)

Does this test hurt to be available? While there is no need for this in dune-common currently, I don't know if some other modules have started to rely in this macro. We can simply have it in dune-common, once we switch to a new compiler, we can set it to 1 by default, and after some time we can add a deprecation, since it is really not needed anymore. Currently, it could be needed by some other module.

changed milestone to %CMake Modernization

changed milestone to %DUNE 2.10.0

I want to come back to the discussion about this MR:

1. Why do you want to make these checks upstream (in dune-common) and not downstream in the modules where it is used? I think we have to assume, that the toolchain stays the same (otherwise we might hit other issues). So, checking either directly in the header where it is defined or used that such utilities must be backported or are available in the compiler/library is also a possibility - maybe the better choice?

2. The Std::identity utility is shipped with libstdc++ and libcxx if c++-20 is available, thus can be deprecated in the near future.

3. The Std::is_detected utilities are shipped with libstdc++ and libcxx for c++>=14. In libstdc++ there is a feature detection macro: #if __cpp_lib_experimental_detect >= 201505, but unfortunately not in libcxx (as far as I can tell). I don't think these utilities will ever be transferred out of the experimental namespace. But concepts do not replace the techniques completely, I think. So, is there any reason to

• provide a backport at all if we could just use the std::experimental implementation from the beginning?
• not implement this utility as a regular utility and ignore any std library TR implementations?

4. For the lambda expression in unevaluated context there is no feature testing macro defined, but the compiler version check should do it as well. It might be that we can remove this check, once we stick to g++ >= 10 and clang >=xy. The correct version to check for clang is not clear to me, though. In cppreference it says full support in clang >= 17, but partial support already in >= 13.

In summary: We could make some decisions on removing/deprecating utilities in the near future. For all the others I suggest to maybe implement a check in code directly (no need for a cmake check) and move it to the header where the utlity is defined. For those where this cannot be done, we should have a check done in downstream (but this is not easily done with the new config.h design). A check in upstream dune-common would be fine as well if we assume that the compiler toolchain does not change.

Regarding the lambda in unevaluated contexts. The test also seems to be insufficient because it does not check that it works with concepts. For example, the code below fails to compile in Apple Clang 15, however, our CMake test happily says that DUNE_HAVE_CXX_UNEVALUATED_CONTEXT_LAMBDA is true.

#include <utility>

template<typename T>
struct AlwaysTrue : public std::true_type {};

template<class E, int I>
concept TestConcept = requires(const E e)
{
  // works: non delayed requirement resolution
  requires true;
  // bug: dependent requirement resolution -> segfault on Apple Clang 15
  requires AlwaysTrue<E>::value;
};

template<class E, int first, int last>
concept ConceptLoop = requires(std::make_integer_sequence<int,last-first> codims)
{
  []<int... c>(std::integer_sequence<int,c...>)
    requires (TestConcept<E,(first+c)> &&...)
  {} (codims);
};

int main() {
   static_assert(ConceptLoop<int, 1, 4>);
}

mentioned in merge request !1334 (merged)

changed milestone to %DUNE 2.11.0

mentioned in issue #379 (closed)