Make libsamplerate work with other libs/frameworks such as JUCE.

[talaviram]

I'm going to make libsamplerate as a JUCE module.

I've made minor "cosmetic" changes that will be very welcomed to keep using the libsamplerate origin git as a submodule of this module.

Explained are the 2 commits:

• Allow compilation under cpp:

  • calloc/malloc in cpp won't compile without casting. sadly due to the JUCE module workflow you need to include 'samplerate.c' in a .cpp file..

• HAVE_CONFIG_H

  • Also without JUCE, if you just link to the library (especially now when it's very open-sourced - thank you!) the generating of 'config.h' just to add it to your code can be tricky and also when not made carefully could conflict with other config.h. with the JUCE module for example I generate all the defs with macros that are already exists in JUCE itself. here is an example of such use.
    https://github.com/WeAreROLI/JUCE/blob/master/modules/juce_audio_formats/codecs/juce_FlacAudioFormat.cpp

[jrlanglois]

Hello fellow JUCEr! Let me know if you need a hand with anything. Totally open to testing if you need it.

[erikd]

C++ is not C. C is not a strict subset of C++.

Adding these casts makes this code incorrect for a C compiler. There may be a way to keep it correct C and improve it for C++ but this is not it.

[talaviram]

Thank you very much for the reply. I really appreciate you've taken the time to look into it.

Adding these casts makes this code incorrect for a C compiler.

I might have missed something but I've checked both calloc for C, C++
and read the following discussion:
https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc

which suggests the only concern might be:

It can hide an error if you forgot to include <stdlib.h>.

I guess it's possible to make something that is more explicit but sure is more redundant. (though it should keep the C code intact):

#ifdef __cplusplus
    if ((psrc = (SRC_PRIVATE *)calloc (1, sizeof (*psrc))) == NULL)
#else
    if ((psrc = calloc (1, sizeof (*psrc))) == NULL)
#endif

[erikd]

The correct solution is to provide a wrapper function that warps all the #ifdef horror in one place and then use the wrapper function everywhere. The wrapper should be a static inline function named psrc_calloc and be placed in common.h.

[erikd]

The function might look something like:

static inline SCR_PRIVATE*
psrc_private_new (void)
{
#ifdef __cplusplus
    return (SRC_PRIVATE *)calloc (1, sizeof (SRC_PRIVATE));
#else
    return calloc (1, sizeof (SRC_PRIVATE));
#endif
}

[talaviram]

Yes. I'll re-commit / force-push my branch for you to review.
Since it has multiple places, cpp's decltype should keep a single inline for all callocs.

update: It seems decltype is C++11 so I'm not sure it's the right way for broader compatibility.

Thank you.

[erikd]

Any C++ fix you supply should be compatible with the broadest possible range of C++ compilers and standards.

[Flamefire]

Adding these casts makes this code incorrect for a C compiler. There may be a way to keep it correct C and improve it for C++ but this is not it.

@erikd Can you provide a source for this claim? IMO casts (the C-style casts used here) are fully valid in C so this should be fine

HAVE_CONFIG_H

@talaviram Your approach is the wrong way round. If you were to go this way use a define like DONT_INCLUDE_CONFIG so the default won't change. Also there is no point in defining this in config.h if this is for including the config.h (so you should include config.h to include config.h?)

A way I'd go with would be to slim down the config.h. Some of the stuff seems to be for tests only -> move to tests. Endianess can be detected with some if-defing (see e.g. Boost). The clipping stuff (CPU_CLIPS_POSITIVE, ...) is sketchy and plain wrong on cross-compilers. I'd do manual clipping there: Check if the float value is outside [INT_MIN, INT_MAX], then cast. Also it assumes the C99 functions are available and no work-around is used which is also wrong. I think I'd just require a C99 compiler and assume the presence of lrint which removes even more toggles.

[erikd]

@Flamefire Saying it adding the casts is "incorrect for C" was probably overstating the case, but with the casts, and without the inclusion of the header file that defines the function that returns a pointer, the C compiler will infer that the functions returns an int which is both wrong and dangerous.

[Flamefire]

and without the inclusion of the header file that defines the function that returns a pointer, the C compiler will infer that the functions returns an int which is both wrong and dangerous.

Do you mean calloc? Not 100% sure this will actually be wrong but at least you can catch it: -Wimplicit-function-declaration for GCC/Clang. The code should anyway be compiled with -Wall (which includes this) and for CI with -Werror. See https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

[erikd]

I like to avoid all un-necessary casts in my code. This is an un-necessary (for C) cast.

[talaviram]

@talaviram Your approach is the wrong way round. If you were to go this way use a define like DONT_INCLUDE_CONFIG so the default won't change. Also there is no point in defining this in config.h if this is for including the config.h (so you should include config.h to include config.h?)

That's a good question. the rationale for HAVE_CONFIG_H is that it's more commonly used. eg https://stackoverflow.com/questions/2438274/meaning-of-dhave-config-h-in-makefiles

[Flamefire]

I like to avoid all un-necessary casts in my code. This is an un-necessary (for C) cast.

Ok understand. I don't understand the need to compile the C code in C++ though (@talaviram ) If this would really be required I'd prefer the cast over the #ifdef code proposed though. It is less error-prone than repeating the code 2 times guarded with an untested #ifdef

[erikd]

The previous code which is 5 lines is pretty fooloproof, but to address you concern, how about this?

static inline SCR_PRIVATE*
psrc_private_new (void)
{
    return
#ifdef __cplusplus
    (SRC_PRIVATE *)
#endif
          calloc (1, sizeof (SRC_PRIVATE));
}

with a comment of why it is like that.

[talaviram]

Since there are 8 places where calloc needs to be modified to support this.
would this be ok?

addition to common.h:

/*
** Adds casting needed if compiled/included within cpp
*/
#ifdef __cplusplus
#define CALLOC(num, size, type)	(type *)calloc(num,size)
#else // __cplusplus
#define CALLOC(num, size, type)	calloc(num,size)
#endif

so all calls would simply be like:
if ((psrc = CALLOC (1, sizeof (*psrc), SRC_PRIVATE)) == NULL)

[Flamefire]

Please don't. You are repeating the type as 2 different ways. I propose:

#ifdef __cplusplus
#define ZERO_ALLOC(type)	static_cast<type*>(calloc(1, sizeof(type)))
#else // __cplusplus
#define ZERO_ALLOC(type)	calloc(1, sizeof(type))
#endif

Or ALLOC_ZEROED.

Any way: Can you explain why exactly this is required? Why would you compile C code with a C++ compiler? This sounds wrong. The whole approach there doesn't look right (especially maintaining cryptography manually is a big red flag). Juce modules should at least support C code natively

There is linuxPackages which could be used here. If this isn't sufficient enough I'd go to the JUCE maintainers and complain enough so they support C modules.

[talaviram]

Thank you for your feedback it is indeed cleaner.
I'll rebase this commit with the suggested feedback.

But what would you do if you provide a single value in cases like the following?
priv = calloc (1, sizeof (*priv) + psrc->channels * sizeof (float)) ;
I think you need to have the ability to define the size...

Can you explain why exactly this is required? Why would you compile C code with a C++ compiler?

JUCE module format cannot include 'c' file.
I do plan to open an issue/contact them through their forum/make a pull-request.

You can see this limitation also here:
https://github.com/WeAreROLI/JUCE/blob/master/modules/juce_audio_formats/codecs/juce_FlacAudioFormat.cpp

You can ask Jules why also other JUCE libs uses this approach:
https://forum.juce.com/t/building-a-juce-module-for-fftw-how-to-deal-with-tons-of-library-sourcefiles/23201

With current module format we provide pre-compiled lib but with the current permissive license of libsamplerate having it as a submodule is cleaner and cross-platform friendly.

[Flamefire]

But what would you do if you provide a single value in cases like the following?

Completely forgot about flexible array members... I guess use your solution then or provide 2 macros where the other looks like CALLOC_REPLACEMENT(type, flex_array_type, num_array_values)

However I still think this is the wrong approach. As @erikd wrote C is not a subset of C++ and if "JUCE module format cannot include 'c' file." then this is a missing feature of JUCE. I seriously doubt the correctness of the used approach. Even in C++ mono-builds (including multiple C++ files in 1) can change semantic. Consider the typedefs (as explained in your second link) or anonymous namespaces (TU local definitions which might conflict). The suggested approach of putting each src in a different namespace is also wrong and will not work.

With current module format we provide pre-compiled lib but with the current permissive license of libsamplerate having it as a submodule is cleaner and cross-platform friendly.

Then it should be real submodules ala CMake add_subdirectory (having the sources unmodified) and be done. Modifying the so-called submodules to push them into a given format is dangerous and wrong. Farthest I'd go would be: Add a pre-generated config.h and tell the C++ compiler to compile all C++ files and the C compiler to compile all C files.

Speaking of config.h: My suggestion would be to change #include <config.h> to #include "config.h" and put a pre-generated config.h into the src folder.

[erikd]

I think the ZERO_ALLOC changes are fine, but I would prefer to avoid having the same #ifdef in nearly every source file.

[erikd]

Having #ifdef` everywhere is a pain in the neck and almost certain to go wrong sooner of later.

Instead, I would suggest that you replace all existing inlcudes of config.h with:

#include "src-config.h"

and then create that file which would just be:

#ifdef HAVE_CONFIG_H
#include "config.h>"
#endif

[Flamefire]

What about my suggestion of including it as "config.h"? You can then place a file named config.h next to the cpp c files including them and be done. No big changes required and guaranteed to work as the current dir is the first to search that include for

[erikd]

config.h is a generated file, generated by autotools. It can't be generated and part of the repo

the cpp files

There are no cpp file in this repo.

[Flamefire]

The author wants to include files of this repo in another. There he could add a config.h if he wants to avoid configuring this project.

[talaviram]

What about my suggestion of including it as "config.h"?

For my specific use case it'll be ok, I can already just put my config.h and set it as a search path.

But HAVE_CONFIG_H is common and I think it's better to not make it mandatory.
This is common practice in many libs -

https://github.com/kikinteractive/libpng/blob/b07750220ef71cb4e943560ef0e60430fe3e63a5/pngpriv.h#L57

https://github.com/renesas-rcar/flac_decoder/blob/master/lib/src/libFLAC/crc.c#L45

(and many more)

[talaviram]

Is there anything else needed for this PR that I am missing?

[erikd]

I would really just like the title of the first commit comment improved. Maybe something like: "Make allocation more C++ friendly".

[talaviram]

Make allocation more C++ friendly

Thanks. I've changed the commit comment.

[Flamefire]

@talaviram I just got reminded of this by something I (re)saw: Compiling this code as C++ is invalid C++ due to the flexible array members being an extension to C++ supported by only some compilers and it is undefined behavior due to missing constructors of objects allocated by malloc instead of new. For an example of correct (but ugly) C++ version see e.g. https://youtu.be/IAdLwUXRUvg?t=1835

It might work, but be aware that it can break at anytime. So I'd strongly suggest to NOT compile this as C++. C++ is not a superset of C