C++ 11 compatibility about daisysp HOT 9 CLOSED

I would be fine using either version, but C++11 is used by existing build system. So I'm not really making a choice in this case.

I've opened this issue because recent commit broke compatibility due to including code that is not even used by library itself. This looks like an unintended regression. So this can be either fixed in library or just by using custom dsp.h implementation that doesn't include those unused functions.

from daisysp.

I don't see any problems with this approach, this difference in behavior won't matter since the functions are not used by library. Maybe add a short comment explaining why it's done like that.

As a solution to more general problem of supporting multiple C++ versions, maybe CI could be configured to make several runs targeting each C++ standard version that would be considered supported? Such list of supported versions doesn't even seem to be even documented/stated anywhere.

from daisysp.

Do you actually need C++11? It's pretty ancient... Sure. Things can be refactored, but why?

from daisysp.

While not currently used, I do see the value of a helper function like this.

I'm in favor of either guarding it by a check for the c++ version, or making it a standard function instead of constexpr on versions less than c++14. Something like:

#if __cplusplus <= 201103L
uint32_t get_next_power2(uint32_t x)
#else
constexpr uint32_t get_next_power2(uint32_t x)
#endif
{
    // function body
    return x;
}

It is a bit messy looking, and leads to different behavior on different c++ versions, but it might be a good solution. If we start doing this more than just here, we should probably make a specific macro for that check.

I can make a PR if everyone likes this idea. I'm also open to other ideas.

from daisysp.

constexpr have copile time optimization benefits if its input isn't const?

from daisysp.

No, but constexpr adds the ability to be evaluated at compile time which can (if used accordingly) have a big impact on the performance. Honestly, I don't understand why constexpr isn't the default.

from daisysp.

@stephenhensley The code could be much tidier like this:

// somewhere in platform.h
#if __cplusplus <= 201103L
#   define D_CONSTEXPR constexpr
#else
#   define D_CONSTEXPR
#endif

// elswhere in daisySP
#include "platform.h"
D_CONSTEXPR uint32_t get_next_power2(uint32_t x)
{
    // function body
    return x;
}

from daisysp.

Introducing a separate preprocessor variable for the sake of this looks a bit excessive to me. Especially coming from someone who advocates abandoning ancient C++ versions ;-)

from daisysp.

Yeah, I think if this is an exception, and not a general rule we can leave it semi-ugly to make it a point that it's not idiomatic with the rest of the library. However, if this becomes a trend with other helpers we can add any necessary preprocessors.

I just opened a PR for this #159 -- I'll give it a quick compile/test on multiple on c++11 in the morning. So feel free to weigh in if anyone thinks anything should change.

from daisysp.