-
Notifications
You must be signed in to change notification settings - Fork 42
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
Comments
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. |
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
Re: msinttypes: I am using that myself for MSVC for my other projects. I assume its license compatible for inclusion? |
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 |
Careful about including other licences, I had to be specific with the 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 |
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. |
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 |
On 3/4/14, chrisisonwildcode [email protected] wrote:
With Bret's malloc branch maturing, we possibly might not need changing |
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. |
On 3/4/14, Bret Curtis [email protected] wrote:
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:
|
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. |
Be aware that programming correctness and performance optimisations don't 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 As for the second, I'm partial to having wm_stdint.h. Reply to this email directly or view it on |
@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_ ? |
On 3/5/14, Bret Curtis [email protected] wrote:
I can happily put the suggestion aside
Not all compiler suites provide a stdint.h (We may decide |
@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 |
On 3/5/14, Bret Curtis [email protected] wrote:
When compiling the lib, that would be OK. But the type changes will |
But then we would need to ship wildmidi_lib.h and our wm_stdint.h right? |
On 3/5/14, Bret Curtis [email protected] wrote:
Yes |
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? |
On 3/5/14, Bret Curtis [email protected] wrote:
I didn't say it's a problem :) |
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. |
On 3/5/14, Bret Curtis [email protected] wrote:
Do you mean something like the following in wildmidi_lib.h #define HAVE_STDINT_H /* meaning we have to generate this header? */ I personally don't like defining system types and Iike to have |
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 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. |
On 3/5/14, Bret Curtis [email protected] wrote:
How do you want to handle the have or have not stdint.h cases? Say we did something like:
[wildmidi_lib.h :]
...
#include
...
Assuming that you mean something like above, say the dev did ... but without defining HAVE_STDINT_H. Then the typedefs will clash. |
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. |
On 3/5/14, Bret Curtis [email protected] wrote:
Consider the attached srcs, try compiling with the following First case is ideal, which is most possibly what will happen when we The second case is the problem. Now, your own compilation really may However, there are systems where int32_t is defined as "[signed] long" http://www.google.com/search?hl=en&source=hp&q=%22typedef+long+int32_t%3B%22 [Attached files possibly won't arrive at github, CC'ing your private mail] O.S. |
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. |
On 3/5/14, Bret Curtis [email protected] wrote:
Yeah, only two ways out: either use our own prefixed types |
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? |
On 3/5/14, Bret Curtis [email protected] wrote:
In our source distributions, we can create a subdirectory like "stdint"
djgpp-v2.04 (officially beta, but used by many) does have stdint.h and
There are those who does have inttypes.h but no stdint.h (can't |
I like this idea better, we provide a reference stdint.h for those that need it. Have a look at my commit here: |
On 3/5/14, Bret Curtis [email protected] wrote:
Is the following good, then?
|
Looks good! |
On 3/5/14, Bret Curtis [email protected] wrote:
OK, will commit to master when I change the types themselves too. Changing the types will probably create conflicts when you want to |
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. |
Merged |
Pushed 518d4e5, closing this. Will update documentation later.
|
@chrisisonwildcode Please help review and validate that the "unsigned long" was indeed uint32_t and not uint64_t (as is normal on modern systems). |
On 3/6/14, Bret Curtis [email protected] wrote:
[unsigned] long use in there can not be uint64_t, otherwise the |
ah... and here: wildmidi_lib.h did you purposely leave int/long etc in for the return values of these functions? |
On 3/6/14, Bret Curtis [email protected] wrote:
int returns are just error state returns, so I saw no reason in long return: we have it with our _GetVersion: not of critical value, unsigned long* param: we have it with _FastSeek(). as I said I first |
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. |
On 3/6/14, Bret Curtis [email protected] wrote:
In that case, the only thing that's worth changing would be
|
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.
The text was updated successfully, but these errors were encountered: