Draft: Make the DUNE_ASSERT_BOUND macro constexpr friendly

Summary

This MR replaces the Dune::RangeError exception by std::out_of_range in the DUNE_ASSERT_BOUND macro. The reason is, that Dune::Exception is not usable in constexpr contexts. A consequence is that now a different type of exception is thrown from operator[] like methods in dune-common.

Note that DUNE_ASSERT_BOUND seems to be used only in dune-common. Thus we could fix this locally. But it is not forbidden to use that macro downstreams, I just have not found any module that uses it.

if I understand correctly the problem is perhaps not so much the use of the macro but code that catches the exception with the current Dune exception type? The is no way of warning about / deprecating this as far as I can see.

Yes, this is the main issues. Throwing the error is not a big problem since it is "irrelevant" to the user what we throw, the the handling of the exception might be problematic. I was thinking whether we could just change the definition of Dune::RangeError to derived from std::out_of_range, but this is also not possible because it changes the interface of the exception. Maybe we could introduce an intermediate level of exception classes that has the same interface (same constructors etc.) as the std::runtime_exceptions and of Dune::Exception but are constexpr friendly. But I am not sure whether this is possible.

I do not yet have an idea of how to deprecate this macro/exception.

I grep'd for catch (const RangeError&) in several dune modules and only dune-istl showed up. But I do not have a "full" set of dune modules to check.

If we want to make our datastructures constexpr, we need some way to fix the DUNE_ASSERT_BOUND macro. We could fix its definition, or replace it with something else, e.g. a generic DUNE_ASSERT macro as proposed elsewhere. The problem is always that the current macro throws a dune specific exception that cannot be made constexpr. Thus a user who catches/handles this exception now gets something else and would need to update her code. Is this something that users do, handling out-of-bound exceptions? Is this an exception that can be handled in a meaningful way, e.g. not just printing an error nessage, or could it be classified as a programming-error with immediate terminate?

mentioned in merge request !1345 (merged)

If I understand it correctly the main issue is, that the DUNE exceptions contain the hook infrastructure. Perhaps we can/should rework it and potentially deprecate it in a way that makes DUNE::Exception constexpr friendly.

Already now the hook-list is not part of the exception, but a static "global" variable. The main problem should be the trigger that is in the constructor, rendering is non-constexpr. We could move the trigger to the DUNE_THROW define, so that the hook is not used if one uses throw Exception("...");, but only with DUNE_THROW(Exception, "...");. We can then still deprecate the usage and remove it in a later release if we consider the feature obsolete (I added it once for parallel debugging, but didn't use it for a long time now..).

I am unsure this is worth the effort. With C++23, "we only have two rules for the requirements of a constexpr function: not a coroutine, and not a constuctor/destructor of a class that has virtual base classes" (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2448r2.html#going-deeper-1)

mentioned in merge request !1401

I don't think I understand well enough how constexpr works, but from my testing, just moving the actual DUNE_THROW to another function makes the compiler happy. Branching with std::is_constant_evaluated didn't work for me. Somehow, my compiler kept finding the instantiation of Dune::Exception during constant evaluation even when the assertion was not met (@christi could you maybe show how you did it?). Below what worked for me. @simon.praetorius could you give this a try? I also want to move forward with the constexpr functions.

namespace Dune {
  void boundCheckThrow()
  {
     DUNE_THROW(Dune::RangeError, "Index out of bounds.");
  }
}

#define DUNE_ASSERT_BOUNDS(cond)                            \
  do {                                                      \
    if (!(cond))                                            \
      Dune::boundCheckThrow();                              \
  } while (false)

See f1a59484.

This additional function does not really help us, as the benefit of DUNE_THROW over throw is, that we get information about the file where the exception was thrown. This is also lost in your version an even more, it gives false information.

The std::is_constant_evaluated had to be used in the #define DUNE_THROW to skip whole construction of the std::ostringstream. In this branch it would not be possible to use the << operator, which might be OK, as we can't collect dynamic information in the constexpr case anyways... I don't this anymore in a branch or something, so I'd have to coo it up again. But this is what I recall from my experiments.

That's a good point. How about just passing information as arguments to the function? See 57988062. How I understand it, the main problem with DUNE_THROW is that it allocates memory that will scape the constexpr context, fortunately __func__ and __FILE__ are just characters that do not need allocation, so they may be used to invoke the function. With 57988062 I get something like this:

Dune::RangeError [boundCheckThrow:/Users/sospinar/Codes/DUNE_COPASI/dune-common/dune/common/boundschecking.hh:26]:
DUNE_ASSERT_BOUNDS [operator[]:/Users/sospinar/Codes/DUNE_COPASI/dune-common/dune/common/fvector.hh:398]: Index out of bounds.

The other thing that was supported by DUNE_THROW, but is not useful for the compile-time checks, is the ability to add a hook. This was useful when debugging parallel code. Perhaps there are nowadays better alternatives.

I have seen this functionality but I never really needed it: It is very easy to catch different parts of error handling mechanisms with catch blocks in gdb, and I do this all the time to find errors. Works like a charm. I am confident that other debuggers also have this or similar features. Otherwise is also easy to set a breaking point at std::exception constructor.

In either case, I think the hook is orthogonal to what I propose above. The hook still gets triggered in non-constant evaluated throws.

mentioned in merge request !1459 (merged)

What about deprecating our own exception and throw macro? Is this location feature (file and line number) really that important that it justifies a Dune-ism? I have to confess, it is handy, especially for people less familiar with C++ and its tooling. On the other hand, I cannot remember when I actually used it myself.

I think several things to this:

1. It's a lot of work to deprecate this for very little benefit. For example, I found 1654 uses in 419 different files on my tree. Take into account that the macro allows to use streams within the second argument. Do you really want to port this?
2. While bound checking does not work now, it just happens to be an unfortunate case where constexpr does not work out of the box pre C++26, in all other places it just works. An alternative seems to be possible from testing.
3. It is quasi useful now, but it may become much more powerful if a proper stack trace is later introduced for better diagnostics.

So I don't really see a point on deprecating it TBH.

1. I don't think having streams in an exception is still desirable. 15 years ago, the C++ world thought streams are a good abstraction. Today, this is much disputed. So, getting rid of our own exception would be worth the effort.
2. Guarding the newly added constexpr with a C++26 macro would enable this, too.
3. Proper stack traces sound nice. I would prefer to get it from standard C++. Unfortunately, the paper got stuck in the process due to low priority, see https://github.com/cplusplus/papers/issues/1056

We could also think about deprecating (and replacing) the usage of DUNE_THROW wherever we want to have constexpr. E.g. in FieldVector::operator[] it would (in my opinion) be fine to throw a std::out_of_range exception, or use the assert macro or something like that. The consequence would be that only for this class, the thrown exception would change and this is something the user needs to be informed about. I have not seen any production code relying on the type of exception in FieldVector, though. (but this is not an argument for replacing it right away)

As discussed here or somewhere else, a completely backwards compatible change of the DUNE_ASSERT_BOUND macro seems either not possible or very hard. However, I have not seen usage of this macro outside of dune-common a lot. @santiago.ospina did your grep for this macro found something in other modules?

Maybe I don't get the point. But the problem only seems to be how the stream support is implemented in the macro. The problem is that a Dune::Exception has a non-trivial destructor and is created as local variable. Directly throwing one as throw Dune::RangeError(); seems to be fine. I didn't check the standard but it works with gcc and clang. In order to get this working we have to put everything int a single expression.

For me the following works:

template<class E>
class ExceptionStream
  : public std::ostringstream
  , public E
{
public:
  ExceptionStream() = default;
  ExceptionStream(const ExceptionStream& other) = delete;
  ExceptionStream(ExceptionStream&& other)
  {
    (*this) << other.str();
  }
  const char* what() const noexcept override
  {
    return this->view().data();
  }
};

#define THROW(E, m) throw ExceptionStream<E>() << THROWSPEC(E) << m;

constexpr int foo(int i) {
  if (i==23)
    THROW( Dune::RangeError, "Foo called with i=" << i);
  // Macro-less throw but without the THROWSPEC prefix
  if (i==237)
    throw Dune::ExceptionStream<Dune::RangeError>() << i;
  return i;
}

Then I can use e.g. std::array<int foo(42)>. Notice that this solution is somewhat incomplete, because it does not provide the hook support and ignores message().

Notice that this solution is somewhat incomplete, because it does not provide the hook support and ignores message().

As far as I can see, the hook is still caught at run time during its construction and the message is guaranteed to be empty after construction. @carsten.graeser could you extend why your idea is incomplete?

It could be that due the hook being static, this is not entirely constexpr (in the sense of using this in try-catch blocks). But as far as I can see, this would solve the non-throwing constexpr path which is the problem raised by this MR.

could you extend why your idea is incomplete?

For two reasons: I never used the hook feature, don't know what it's for, and didn't check if the proposed solutions properly supports it. Furthermore, while the classical use case does what it's supposed to do, it does not respect the internal message member. If someone manually sets it using e.message("foo"), this will be ignored.

This solution is for the macro right? The exception is constructed within the macro, so there is no-one manually setting e.message(“foo”) between exception construction its throw statement. Or I just don’t understand where this extra message could be placed.

But people can access this string using Dune::Exception::what(). While it's very unlikely, that this is done manually, people may capture the exception by value. The the message set via the derived stream class would be lost due to slicing.

But this solution probably does the trick and is much cleaner:

template<class E>
class ExceptionStream
  : public E
{
public:

  template<class T>
    requires requires(std::ostringstream& oss, T t) { oss << t; }
  friend ExceptionStream& operator<<(ExceptionStream& es, const T& t)
  {
    es.sstream_ << t;
    es.message(es.sstream_.str());
    return es;
  }

  template<class T>
    requires requires(std::ostringstream& oss, T t) { oss << t; }
  friend ExceptionStream operator<<(ExceptionStream&& es, const T& t)
  {
    es << t;
    return es;
  }

private:
  std::ostringstream sstream_;
};

#define THROW(E, m) throw ExceptionStream<E>() << THROWSPEC(E) << m;

Cf. !1472 (merged) for an implementation of DUNE_THROW using the proposed approach.

Great, thank you!

!1472 (merged) revealed a few downstream bugs that should now all be fixed and the full-system-test pipeline passes.

@simon.praetorius Can you check if !1472 (merged) solves your problem, too?

I created a patch of the branch in MR !1401, see branch issue/fieldvector-1-constructors-3, that makes the operator[] in FieldVector constexpr using DUNE_ASSERT_BOUND, which uses DUNE_THROW. It seems to work: https://gitlab.dune-project.org/core/dune-common/-/pipelines/75610

Thanks for the feedback.

mentioned in merge request !1472 (merged)

Will be closed in favor of !1472 (merged)

closed