Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace 'long' type usage by int or int32_t #43

Closed
sezero opened this issue Mar 2, 2014 · 42 comments
Closed

replace 'long' type usage by int or int32_t #43

sezero opened this issue Mar 2, 2014 · 42 comments

Comments

@sezero
Copy link
Contributor

sezero commented Mar 2, 2014

There are many places data are long typed where they can actually be int, i.e. int32_t. See issue #24 where valgrind reports about 220mb total malloc'ed memory on an x86 but about 440mb on an x64.

A decision should be made whether to use plain 'int' or int32_t, or define our own 32bit type based on _LP64 definition and/or limits.h values.

This may be a v0.4-only material: not sure how much of the public api would be affected.

@psi29a
Copy link
Member

psi29a commented Mar 2, 2014

We will need something portable, I prefer the int32_t method, but that isn't standard in Visual Studio (even 2012). We would need to include this: https://code.google.com/p/msinttypes/ for that support.

@sezero
Copy link
Contributor Author

sezero commented Mar 2, 2014

How about something like the following:

diff --git a/include/wildmidi_lib.h b/include/wildmidi_lib.h index 63d1ed0..a4903c9 100644 --- a/include/wildmidi_lib.h +++ b/include/wildmidi_lib.h @@ -70,6 +70,29 @@ # define WM_SYMBOL #endif

+/* data types /
+#ifdef HAVE_STDINT_H
+#include <stdint.h>
+typedef int8_t WM_int8_t;
+typedef uint8_t WM_uint8_t;
+typedef int16_t WM_int16_t;
+typedef uint16_t WM_uint16_t;
+typedef int32_t WM_int32_t;
+typedef uint32_t WM_uint32_t;
+#else
+/
these must be correct on any sane system /
+typedef signed char WM_int8_t;
+typedef unsigned char WM_uint8_t;
+typedef signed short int WM_int16_t;
+typedef unsigned short int WM_uint16_t;
+typedef signed int WM_int32_t;
+typedef unsigned int WM_uint32_t;
+/
compile time asserts for type sizes: _/
+typedef int _WM_int8_test [(sizeof(WM_int8_t ) == 1) * 2 - 1];
+typedef int _WM_int16_test[(sizeof(WM_int16_t) == 2) * 2 - 1];
+typedef int WM_int32_test[(sizeof(WM_int32_t) == 4) * 2 - 1];
+#endif /
HAVE_STDINT_H */
+
#if defined(__cplusplus)
extern "C" {
#endif

Re: msinttypes: I am using that myself for MSVC for my other projects. I assume its license compatible for inclusion?

@psi29a
Copy link
Member

psi29a commented Mar 2, 2014

msinttypes has a BSD license: http://opensource.org/licenses/BSD-3-Clause

So we can include it, but we would need include the license in /docs/license

@chrisisonwildcode
Copy link
Contributor

Careful about including other licences, I had to be specific with the
documentation otherwise it would not of been able to be included in some
distros.

I would be cautious about mixing licences.

Chris

Sent from my iPhone

On 2 Mar 2014, at 9:02 pm, Bret Curtis [email protected] wrote:

msinttypes has a BSD license: http://opensource.org/licenses/BSD-3-Clause

So we can include it, but we would need include the license in /docs/license

Reply to this email directly or view it on
GitHubhttps://github.com//issues/43#issuecomment-36451374
.

@psi29a
Copy link
Member

psi29a commented Mar 2, 2014

No distro on this planet will exclude wildmidi because it included BSD licensed code.

The Free Software Foundation, which refers to the license as the "Modified BSD License", states that it is compatible with the GNU GPL.

thus:

This version allows unlimited redistribution for any purpose as long as its copyright notices and the license's disclaimers of warranty are maintained. The license also contains a clause restricting use of the names of contributors for endorsement of a derived work without specific permission.

@chrisisonwildcode
Copy link
Contributor

As I go through some of the changes you guys are making some things are coming back to me.

The reason I used long int instead of just int is that some older systems an int can be 16bit and not 32bit (may be still so in some cases) and defining it as a long int told the compiler I wanted 32bit int without having to do system dependant stuff. This was because around 10 years ago there were still systems that int was 16bit on.

With the advent of 64bit systems, int can now be anything from 16bit to 64bit depending on the compiler used (apparently). Defining it as short or long (or long long for 64bit) ensures better compatibility without having to go system specific.

http://www.agner.org/optimize/calling_conventions.pdf (chapter 3) ... and I see from that table that 64bit GNU/Intel Linux long int = 64bit ... grrr

@sezero
Copy link
Contributor Author

sezero commented Mar 4, 2014

On 3/4/14, chrisisonwildcode [email protected] wrote:

As I go through some of the changes you guys are making some things are
coming back to me.

The reason I used long int instead of just int is that some older systems an
int can be 16bit and not 32bit (may be still so in some cases) and defining
it as a long int told the compiler I wanted 32bit int without having to do
system dependant stuff. This was because around 10 years ago there were
still systems that int was 16bit on.

With the advent of 64bit systems, int can now be anything from 16bit to
64bit depending on the compiler used (apparently). Defining it as short or
long (or long long for 64bit) ensures better compatibility without having to
go system specific.

With Bret's malloc branch maturing, we possibly might not need changing
'long' to int32_t, much. On an amd64-linux system, doing so reduces the
total heap usage 494mb to 290mb with HEAD branch, but with the malloc
branch the change would only shaves about 317kb, i.e. 2729kb vs 2413kb.

@psi29a
Copy link
Member

psi29a commented Mar 4, 2014

I think, in interest of preserving Chris' intent, we should use the "int32_t" nomenclature. That way, we are consistent on all platforms that we intended to use that specific type. If that type doesn't exist, we automatically upgrade to the next usable type.

I see it more as stamping into the code specifically what we want. This makes it easier to change later.

@sezero
Copy link
Contributor Author

sezero commented Mar 4, 2014

On 3/4/14, Bret Curtis [email protected] wrote:

I think, in interest of preserving Chris' intent, we should use the
"int32_t" nomenclature. That way, we are consistent on all platforms that we
intended to use that specific type. If that type doesn't exist, we
automatically upgrade to the next usable type.

I see it more as stamping into the code specifically what we want. This
makes it easier to change later.

OK then. Here are the typedefs I plan on adding:

#ifndef WILDMIDI_STDINT
#define WILDMIDI_STDINT
#ifdef HAVE_STDINT_H
# include 
  typedef int8_t        WM_SINT8;
  typedef uint8_t       WM_UINT8;
  typedef int16_t       WM_SINT16;
  typedef uint16_t      WM_UINT16;
  typedef int32_t       WM_SINT32;
  typedef uint32_t      WM_UINT32;
# else /* HAVE_STDINT_H */
  typedef signed char       WM_SINT8;
  typedef unsigned char     WM_UINT8;
  typedef signed short int  WM_SINT16;
  typedef unsigned short int    WM_UINT16;
# include 
# if (INT_MAX == 2147483647)
  typedef signed int        WM_SINT32;
  typedef unsigned int      WM_UINT32;
# elif (LONG_MAX == 2147483647)
  typedef signed long int   WM_SINT32;
  typedef unsigned long int WM_UINT32;
# else
#error Define a 32bit integral type
#endif /* int32 */
#endif /* HAVE_STDINT_H */
typedef int _wm_int8_test [(sizeof(WM_SINT8 ) == 1)*2 -1];
typedef int _wm_int16_test[(sizeof(WM_SINT16) == 2)*2 -1];
typedef int _wm_int32_test[(sizeof(WM_SINT32) == 4)*2 -1];
#endif /* WILDMIDI_STDINT */

Here are a few points to decide:

  • Only change long to WM_?INT32, or change short and relevant char
    to WM_?INT?? too?
  • Where to include the types? Naturally, wildmidi_lib.h is the
    place, however not all sources include it during library build.
    One solution is duplicating them in common.h, another is creating
    a new header like wm_stdint.h and put things there which requires
    distributing along with wildmidi_lib.h.

@psi29a
Copy link
Member

psi29a commented Mar 4, 2014

I would prefer to change them all, buffers should be unsigned char and then casted for use (for example).

As for the second, I'm partial to having wm_stdint.h.

@chrisisonwildcode
Copy link
Contributor

Be aware that programming correctness and performance optimisations don't
always mix. Changing the mix buffer to a char and recasting as needed can
create excessive overhead in the main processing loops of WM_GetOutput_* .
Recasting should be done as little as possible for performance purposes.

Chris

Sent from my iPhone

On 5 Mar 2014, at 9:00 am, Bret Curtis [email protected] wrote:

I would prefer to change them all, buffers should be unsigned char and then
casted for use (for example).

As for the second, I'm partial to having wm_stdint.h.

Reply to this email directly or view it on
GitHubhttps://github.com//issues/43#issuecomment-36690001
.

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

@chrisisonwildcode this will have to be tested, but I suspect that you are correct. I guess we should leave things as they are (type I mean). And only do conversions on a case by case basis. What might work with no-cost on one platform might be costly on another.

@sezero Why not use the standard stdint nominclature instead of inventing our own WM__INT_ ?

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

@chrisisonwildcode this will have to be tested, but I suspect that you are
correct. I guess we should leave things as they are (type I mean). And only
do conversions on a case by case basis. What might work with no-cost on one
platform might be costly on another.

I can happily put the suggestion aside

@sezero Why not use the standard stdint nominclature instead of inventing
our own WM__INT_ ?

Not all compiler suites provide a stdint.h (We may decide
that we support only those who does, but that would be overkill.)

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

@sezero I meant, use stdint.h and if stdint.h doesn't exist, then define the XintX_t ourselves. This removes the need for WM_X

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

@sezero I meant, use stdint.h and if stdint.h doesn't exist, then define the
int__t ourselves. This removes the need for WM*

When compiling the lib, that would be OK. But the type changes will
affect the exported api in wildmidi_lib.h, and no one can know how
the user of that header has a check for stdint.h on anything, so this
is the only foolproof way I know that it will work for everyone.

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

But then we would need to ship wildmidi_lib.h and our wm_stdint.h right?

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

But then we would need to ship wildmidi_lib.h and our wm_stdint.h right?

Yes

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

Then how is this a problem? wm_stdint.h is to be #include into wildmidi_lib.h, then we're safe and so are our end users?

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

Then how is this a problem? wm_stdint.h is to be #include into
wildmidi_lib.h, then we're safe and so are our end users?

I didn't say it's a problem :)

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

I mean, why do we then need WM_X and not the standard XintX_t ? If their systems do not have stdint.h, fine, we define the XintX_t ourselves.

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

I mean, why do we then need WM_X and not the standard XintX_t ? If their
systems do not have stdint.h, fine, we define the XintX_t ourselves.

Do you mean something like the following in wildmidi_lib.h

#define HAVE_STDINT_H /* meaning we have to generate this header? */
#ifdef HAVE_STDINT_H
#include <stdint.h>
#else
typedef short int16_t;
[other types...]
#endif

I personally don't like defining system types and Iike to have
a standart foolproof header that can be used across platforms.

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

Yes, that way.

I just don't see how this way or the other way is any more foolproof than the other, aside that this way is more to the point. IMO, adding yet another layer of abstraction, above stdint.h typedefs isn't gaining us anything... quite the opposite.

If stdint.h exists, then uint8_t exists. C99
if stdint.h doesn't exist, then we define uint8_t to exist and match it to unsigned char. non-C99

uint8_t isn't a system type, it is just an explicitly set typedef to a system type, we just try our best to match it to our desired data type... those nasty int, long and long longs. ;)

At least we can be C99 compatible, but making it not required.

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

Yes, that way.

I just don't see how this way or the other way is any more foolproof than
the other, aside that this way is more to the point. IMO, adding yet another
layer of abstraction, above stdint.h typedefs isn't gaining us anything...
quite the opposite.

If stdint.h exists, then uint8_t exists. C99
if stdint.h doesn't exist, then we define uint8_t to exist and match it to
unsigned char. non-C99

How do you want to handle the have or have not stdint.h cases?

Say we did something like:

[wildmidi_lib.h :] ... #include ...

[wm_stdint.h :]
#ifndef WM_STDINT_H
#define WM_STDINT_H

#ifdef HAVE_STDINT_H

include <stdint.h>

else /* HAVE_STDINT_H */

typedef signed char int8_t;
[...blah...]

Assuming that you mean something like above, say the dev did
something like this:

#include <stdint.h>
...
#include <wildmidi_lib.h>

... but without defining HAVE_STDINT_H. Then the typedefs will clash.

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

Inside stdint.h is this:

#ifndef _STDINT_H
#define _STDINT_H       1
...
#endif

The clash wouldn't happen. If stdint.h is called before wildmidi_lib.h, then STDINT_H is defined and when it gets to wildmidi_lib, it will also try to include stdint.h, which is guarded.

On VS, it also has a guard.

#ifndef _MSC_STDINT_H_ // [
#define _MSC_STDINT_H_

This is be the first rule in defensive programming, guard your includes.

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

Inside stdint.h is this:

#ifndef _STDINT_H
#define _STDINT_H       1
...
#endif

The clash wouldn't happen. If stdint.h is called before wildmidi_lib.h, then
_STDINT_H is defined and when it gets to wildmidi_lib, it will also try to
include stdint.h, which is guarded.

Consider the attached srcs, try compiling with the following
two cmdlines:
gcc -I. -DHAVE_STDINT_H -c src.c
gcc -I. -c src.c

First case is ideal, which is most possibly what will happen when we
ourselves are compiling the lib using our cmake'ry.

The second case is the problem. Now, your own compilation really may
succeed, but that will be because your system stdint.h possibly really
is defining int32_t as "[signed] int" and int8_t as "signed char",
i.e. no clashes.

However, there are systems where int32_t is defined as "[signed] long"
even if int itself is 32 bits, and there are systems where int8_t is
defined as "char" and not as "signed char": with those ones, our header
will fail. You cannot trust that all will be OK because everybody
else will be behaving.

http://www.google.com/search?hl=en&source=hp&q=%22typedef+long+int32_t%3B%22
http://www.google.com/search?hl=en&source=hp&q=%22typedef+char+int8_t%3B%22

[Attached files possibly won't arrive at github, CC'ing your private mail]

O.S.

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

I follow your logic, you mean if other people also do not have stdint.h and do their own implementation which would conflict with our own. To avoid collision, you suggest prefixing the types with wm_

ugh... at this point, I would rather say screw it, and force C99 down everyone's throat. We do not work with systems without stdint.h. ;)

^-- said out of frustration. Your solution is still a better one.

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

I follow your logic, you mean if other people also do not have stdint.h and
do their own implementation which would conflict with our own. To avoid
collision, you suggest prefixing the types with wm_

ugh... at this point, I would rather say screw it, and force C99 down
everyone's throat. We do not work with systems without stdint.h. ;)

Yeah, only two ways out: either use our own prefixed types
(virtually all libs out there do it, e.g. sdl, ogg, flac, etc, etc.),
or force users to have stdint.h which I am not uncomfortable with.

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

Indeed... we strive to be cross-platform, that also means (sadly) on systems without stdint.h

Do we draw a line and say, only platforms with C99 support?

How will djgpp handle this, or is this a not a problem? I know that all my platforms that I've tested on has stdint.h but I'm not sure about others?

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

Indeed... on one hand, we strive to be cross-platform, that also means
(sadly) on systems without stdint.h

Do we draw a line and say, only platforms with C99 support?

In our source distributions, we can create a subdirectory like "stdint"
and place a custom implementation in there to help developers

How will djgpp handle this, or this a no-brainer?

djgpp-v2.04 (officially beta, but used by many) does have stdint.h and
inttypes.h. For those who prefer v2.03, they can use our custom version

I know that all my
platforms that I've tested on has stdint.h but I'm not sure about others?

There are those who does have inttypes.h but no stdint.h (can't
remember exactly which ones they are, but they are there.)

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

I like this idea better, we provide a reference stdint.h for those that need it.

Have a look at my commit here:
zinnschlag/openmw#1276 (comment)
I told cmake to look for stdint.h, it not found, then die with a message.

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

I like this idea better, we provide a reference stdint.h for those that need
it.

Have a look at my commit here:
zinnschlag/openmw#1276 (comment)
I told cmake to look for stdint.h, it not found, then die with a message.

Is the following good, then?


diff --git a/CMakeLists.txt b/CMakeLists.txt
index dd30f4e..69f90c2 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -108,6 +108,9 @@ CHECK_C_SOURCE_COMPILES("static __inline__ int
static_foo() {return 0;}
 CHECK_C_SOURCE_COMPILES("static __inline int static_foo() {return 0;}
                          int main(void) {return 0;}" HAVE_C___INLINE)
+CHECK_INCLUDE_FILE(stdint.h HAVE_STDINT_H)
+CHECK_INCLUDE_FILE(inttypes.h HAVE_INTTYPES_H)
+
 TEST_BIG_ENDIAN(WORDS_BIGENDIAN)
 SET(AUDIODRV_ALSA)
@@ -228,6 +231,9 @@ ENDIF(WIN32)
 INCLUDE_DIRECTORIES(BEFORE "${CMAKE_SOURCE_DIR}/include")
 FILE (MAKE_DIRECTORY "${CMAKE_BINARY_DIR}/include")
 INCLUDE_DIRECTORIES(BEFORE "${CMAKE_BINARY_DIR}/include")
+IF(NOT HAVE_STDINT_H) # AND NOT HAVE_INTTYPES_H
+   INCLUDE_DIRECTORIES(BEFORE "${CMAKE_SOURCE_DIR}/include/stdint")
+ENDIF()
 IF(APPLE)
     SET(APP_BUNDLE_NAME "${CMAKE_PROJECT_NAME}.app")
diff --git a/include/config.h.cmake b/include/config.h.cmake
index 0ad435a..a0c4ac5 100644
--- a/include/config.h.cmake
+++ b/include/config.h.cmake
@@ -49,6 +49,12 @@
 /* define this if you are running a bigendian system (motorola, sparc, etc) */
 #cmakedefine WORDS_BIGENDIAN 1
+/* Define if you have the  header file. */
+#cmakedefine HAVE_STDINT_H
+
+/* Define if you have the  header file. */
+#cmakedefine HAVE_INTTYPES_H
+
 /* Define our audio drivers */
 #cmakedefine HAVE_LINUX_SOUNDCARD_H
 #cmakedefine HAVE_SYS_SOUNDCARD_H
diff --git a/src/file_io.c b/src/file_io.c
index 24e1508..5904c75 100644
--- a/src/file_io.c
+++ b/src/file_io.c
@@ -26,6 +26,7 @@
 #include "config.h"
+#include 
 #include 
 #include 
 #include 
diff --git a/src/gus_pat.c b/src/gus_pat.c
index ea4a5c1..c270105 100644
--- a/src/gus_pat.c
+++ b/src/gus_pat.c
@@ -26,6 +26,7 @@
 #include "config.h"
+#include 
 #include 
 #include 
 #include 
diff --git a/src/reverb.c b/src/reverb.c
index 47d8021..dd424c3 100644
--- a/src/reverb.c
+++ b/src/reverb.c
@@ -26,6 +26,7 @@
 #include "config.h"
+#include 
 #include 
 #include 
diff --git a/src/wildmidi.c b/src/wildmidi.c
index c0552e8..0e72252 100644
--- a/src/wildmidi.c
+++ b/src/wildmidi.c
@@ -26,6 +26,7 @@
 #include "config.h"
+#include 
 #include 
 #include 
 #include 
diff --git a/src/wildmidi_lib.c b/src/wildmidi_lib.c
index 86a6a55..914dcc7 100644
--- a/src/wildmidi_lib.c
+++ b/src/wildmidi_lib.c
@@ -28,6 +28,7 @@
 #define UNUSED(x) (void)(x)
+#include 
 #include 
 #include 
 #include 
diff --git a/include/stdint/stdint.h b/include/stdint/stdint.h
new file mode 100644
index 0000000..34870ea
--- /dev/null
+++ b/include/stdint/stdint.h
@@ -0,0 +1,27 @@
+#ifndef WM_STDINT_H
+#define WM_STDINT_H
+
+#include 
+
+typedef signed char int8_t;
+typedef unsigned char uint8_t;
+typedef signed short int int16_t;
+typedef unsigned short int uint16_t;
+
+#if (INT_MAX == 2147483647)
+typedef signed int int32_t;
+typedef unsigned int uint32_t;
+#elif (LONG_MAX == 2147483647)
+typedef signed long int int32_t;
+typedef unsigned long int uint32_t;
+#else
+#error define a 32bit integral type
+#endif /* int32_t */
+
+/* make sure of type sizes */
+typedef int _wm_int8_test [(sizeof(int8_t ) == 1) * 2 - 1];
+typedef int _wm_int16_test[(sizeof(int16_t) == 2) * 2 - 1];
+typedef int _wm_int32_test[(sizeof(int32_t) == 4) * 2 - 1];
+
+#endif /* WM_STDINT_H */
+

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

Looks good!

@sezero
Copy link
Contributor Author

sezero commented Mar 5, 2014

On 3/5/14, Bret Curtis [email protected] wrote:

Looks good!

OK, will commit to master when I change the types themselves too.
Won't change 0.3 and keep it to 0.4-only.

Changing the types will probably create conflicts when you want to
merge the malloc branch: how do you want to proceed?

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

I'll merge the malloc branch Thursday morning and be done with it. There are certainly more optimizations that can be done, but we've taken care of all the really heinous memory abusing code. Let us keep both of these in 0.4/master.

@psi29a
Copy link
Member

psi29a commented Mar 5, 2014

Merged

@sezero
Copy link
Contributor Author

sezero commented Mar 6, 2014

Pushed 518d4e5, closing this. Will update documentation later.

  • Please check the changeset line by line: it's easy to make
    a mistake.
  • This results in api changes documented in the README,
    hopefully without missing anything: please check.
  • Didn't change WildMidi_FastSeek() which takes an
    unsigned long* sample_pos: first I thought that it would make
    dev's life easier to port over code, but there are enough api
    changes, so if you say that we change it to
    "uint32_t* sample_pos" I will do it right away.

@sezero sezero closed this as completed Mar 6, 2014
@psi29a
Copy link
Member

psi29a commented Mar 6, 2014

@chrisisonwildcode Please help review and validate that the "unsigned long" was indeed uint32_t and not uint64_t (as is normal on modern systems).

@sezero
Copy link
Contributor Author

sezero commented Mar 6, 2014

On 3/6/14, Bret Curtis [email protected] wrote:

@chrisisonwildcode Please help review and validate that the "unsigned long"
was indeed uint32_t and not uint64_t (as is normal on modern systems).

[unsigned] long use in there can not be uint64_t, otherwise the
whole thing would have failed miserably on 32bit systems.

@psi29a
Copy link
Member

psi29a commented Mar 6, 2014

ah... and here: wildmidi_lib.h

did you purposely leave int/long etc in for the return values of these functions?

@sezero
Copy link
Contributor Author

sezero commented Mar 6, 2014

On 3/6/14, Bret Curtis [email protected] wrote:

ah... and here: wildmidi_lib.h

did you purposely leave int/long etc in for the return values of these
functions?

int returns are just error state returns, so I saw no reason in
changing them.

long return: we have it with our _GetVersion: not of critical value,
but I can change it.

unsigned long* param: we have it with _FastSeek(). as I said I first
thought it may help with porting if I don't change that to uint32_t*:
all other exported api changes are somewhat "harmless" to the dev,
i.e. one can pass an unsigned long size param to _OpenBuffer() and it
would be silently cast to uint32_t and just work fine, but with the
_FastSeek case it is a pointer and may result in unpredicted stuff.
Then again, such a thing would be a porting error on the dev's side,
and I am totally OK with changing it if you want.

@psi29a
Copy link
Member

psi29a commented Mar 6, 2014

I say we go for a big-bang release... we're in the process of stabilizing and have added a new feature. Downstream then has a choice, either stay with 0.3 branch or put the effort into updating to 0.4

I would rather be consistent all the way through.

I'll be making sure Thirdeye plays nicely and I'll be helping gstreamer as well.

@sezero
Copy link
Contributor Author

sezero commented Mar 6, 2014

On 3/6/14, Bret Curtis [email protected] wrote:

I say we go for a big-bang release... we're in the process of stabilizing
and have added a new feature. Downstream then has a choice, either stay
with 0.3 branch or put the effort into updating to 0.4

I would rather be consistent all the way through.

In that case, the only thing that's worth changing would be
the _FastSeek procedure: its sample_pos param is treated as
uint32_t internally. (Keeping it as unsigned long* only would
help with old code..)

I'll be making sure Thirdeye plays nicely and I'll be helping gstreamer as
well.

@sezero sezero mentioned this issue Mar 8, 2014
@psi29a psi29a added this to the 0.4 - Now with more support! milestone Jan 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants