Fix compile error for non-void-returning inplace_functions

[ssilverman]

The error occurs when testing for or calling an unassigned function.

Here's a test program:

stdext::inplace_function<bool()> func;

void setup() {
  Serial.begin(115200);
  while (!Serial && millis() < 4000) {
    // Wait for Serial
  }

  // Either this:
  func();

  // Or this will cause the issue:
  if (func == nullptr) {
    printf("Here\r\n");
  }
}

void loop() {
}

[ssilverman]

I just changed std::abort() to std::__throw_bad_function_call() because that's how std::function behaves. We could always change back to std::abort() if that's preferred, to avoid whatever std::terminate() (called by that throwing function) sends to stderr (if it even does...). I just felt it was good practice to do whatever std::function does because why not make the behaviours match?

(Note: I believe std::abort() ultimately calls the abort() function defined in startup.c, and I know that doesn't exit, as desired.)

[ssilverman]

Here's another reason for why this change is appropriate: If inplace_function ever becomes part of the standard library, I'd bet that its behaviour will do the same thing as this change. Therefore, future behaviour won't change for the same code.

[ssilverman]

Another few comments:

1. I've placed the definition of the SG14_INPLACE_FUNCTION_THROW macro inside the #ifndef so that it can be externally defined if desired.
2. The latest version is as close to the original source as possible.

[PaulStoffregen]

Looks like this would change the default behavior from no operation to stopping the user's program.

[ssilverman]

Looks like this would change the default behavior from no operation to stopping the user's program.

This is how std::function behaves; the program stops. Also, there isn’t already an established “default behaviour.”

I’m not sure how else to solve the compile error without stopping the program. Did you try the example program?

[PaulStoffregen]

We're not adopting std::function exception behavior.

The intention for callbacks on Teensy is that uninitialized callbacks are "safe" and do nothing if called anyway. Yes, this is a departure from C++ std::function convention where an exception is thrown. It is an intentional choice.

[ssilverman]

Looks like this would change the default behavior from no operation to stopping the user's program.

There’s a choice here:

1. inplace_function doesn’t compile for non-void-returning functions and so is not usable.
2. It works for all functions, even non-void-returning functions. In this case, the program must halt at the location of my change.
3. Make some modifications to the file (eg. adding another vtable() function) so that non-void-returning functions compile.

Edit: I was wrong about not being able to compile for void returns if there is no halt. Paul proposed a working solution below.

[PaulStoffregen]

Perhaps this would solve the problem?`

#define SG14_INPLACE_FUNCTION_THROW(x) return static_cast<R>(0)

[ssilverman]

We're not adopting std::function exception behavior.

The intention for callbacks on Teensy is that uninitialized callbacks are "safe" and do nothing if called anyway. Yes, this is a departure from C++ std::function convention where an exception is thrown. It is an intentional choice.

I don’t disagree with that decision, however, as of right now, the file doesn’t compile. If you don’t want to “throw”, what about calling “abort”?

[ssilverman]

Perhaps this would solve the problem?`

#define SG14_INPLACE_FUNCTION_THROW(x) return static_cast<R>(0)

That might work, but I'm not sure about the general case. Could we at least put this inside an #ifndef (similar to what this PR has), so that I could define that macro myself?

[PaulStoffregen]

I'm reluctant to make this configurable, since it's included by Arduino.h.

And as a general trend, on the forum we're seeing the most common problem transition from power-only USB cables to people using slightly different configurations or tools (which often they don't disclose until many messages digging into the problem) that cause their implementation of the core library to differ in unexpected ways.

I want to enable people to experiment, but at the same time we the platform to be stable and predictable for the sake of support.

[ssilverman]

Perhaps this would solve the problem?`

#define SG14_INPLACE_FUNCTION_THROW(x) return static_cast<R>(0)

This solution means we're returning 0 from a void function, for the case where the function returns void. But it looks like it compiles...

[PaulStoffregen]

Please give return static_cast<R>(0) a try. I ran a few quick tests here. It seems to solve the problem and preserves the intended "no operation" behavior.

[ssilverman]

Please give return static_cast<R>(0) a try. I ran a few quick tests here. It seems to solve the problem and preserves the intended "no operation" behavior.

Looks like it compiles fine for void as well as bool.

[PaulStoffregen]

This solution means we're returning 0 from a void function

Must confess, I'm not a C++ template expert... but info I've been able to find says that use of static_cast can convert to void type, so we're not really returning 0 in the void case.

[ssilverman]

This solution means we're returning 0 from a void function

Must confess, I'm not a C++ template expert... but info I've been able to find says that use of static_cast can convert to void type, so we're not really returning 0 in the void case.

From here: https://en.cppreference.com/w/cpp/language/static_cast:

4. If target-type is the type void (possibly cv-qualified), static_cast discards the value of expression after evaluating it.

I'll update this PR...

[luni64]

The intention for callbacks on Teensy is that uninitialized callbacks are "safe" and do nothing if called anyway. Yes, this is a departure from C++ std::function convention where an exception is thrown. It is an intentional choice.

If I understand correctly, this is not about unitialized callbacks. Actually you are checking for those in pit_isr. So that can't go wrong I'd say. I can not imagine how one would run into the SG14_INPLACE_FUNCTION_THROW macro with intervalTimer.

The issue is, that the current SG14_INPLACE_FUNCTION_THROW macro only compiles for functions returning void,. inplace_function can be used for much more than simple callbacks. IMHO it would be a pitty if we would reduce the capability of this very useful class without need.

Edit: just saw that you found a solution :-) Ignore my late comment...

[ssilverman]

The intention for callbacks on Teensy is that uninitialized callbacks are "safe" and do nothing if called anyway. Yes, this is a departure from C++ std::function convention where an exception is thrown. It is an intentional choice.

If I understand correctly, this is not about unitialized callbacks. Actually you are checking for those this in pit_isr. So that can't go wrong I'd say. I can not imagine how one would run into the SG14_INPLACE_FUNCTION_THROW macro with intervalTimer.

The issue is, that the current SG14_INPLACE_FUNCTION_THROW macro only compiles for functions returning void,. inplace_function can be used for much more than simple callbacks. IMHO it would be a pitty if we would reduce the capability of this very useful class without need.

I'm curious, @luni64, what would be your personal preferences for these (obviously @PaulStoffregen is the final arbiter, I was just curious about your opinions, even if they differ):

1. SG14_INPLACE_FUNCTION_THROW being defined inside an #ifndef or not?
2. Stopping or "returning zero" (static_cast<R> to zero)?

Note: I just force-pushed to make the test program compile and to keep in line with Paul's wishes.

[luni64]

I'm curious, @luni64, what would be your personal preferences

I'd vote for an abort/crash in whatever form, Trying to call a function through a non initialized inplace_function variable is conceptually the same as trying to call a function through a non initialized function pointer variable. The latter will cause some unpredictable behaviour. For the former we can at least generate a defined crash / crashreport.

I don't see why a program should continue running if one tries to call a function with undefined adress. It would only hide a bug.

[ssilverman]

I'm curious, @luni64, what would be your personal preferences

I'd vote for an abort/crash in whatever form, Trying to call a function through a non initialized inplace_function variable is conceptually the same as trying to call a function through a non initialized function pointer variable. The latter will cause some unpredictable behaviour. For the former we can at least generate a defined crash / crashreport.

I don't see why a program should continue running if one tries to call a function with undefined adress. It would only hide a bug.

That would be my vote too. I strongly agree with you.

[PaulStoffregen]

Looks like we're going to continue this conversation on #719, so I'm locking this closed issue.