We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

RE: Title:  not so small a patch..

* effect_position.c, effect_stereoreverse.c:  For changes like:
-   len % sizeof (Uint32) != 0
+   (size_t)len % sizeof (Uint32) != 0

... it would be easier to do:
-   len % sizeof (Uint32) != 0
+   len % (int)sizeof(Uint32) != 0

... provided that len is int type (they all are int type, AFAICS.)
What do you think?

OTOH, effect_position.c line #1520 change is Ok.


* load_voc.c:
- sblen = ((bits24[0]) | (bits24[1] << 8) | (bits24[2] << 16));
+ sblen = (Uint32)((bits24[0]) | (bits24[1] << 8) | (bits24[2] << 16));

I _think_ it would be more correct doing like the following?
  sblen = (Uint32)(bits24[0]) | ((Uint32)bits24[1] << 8) | ((Uint32)bits24[2] << 16);

Same for music_mad.c changes of the like.


* load_voc.c, voc_read():
The procedure doesn't return negative. So, instead of casting done
to (int), changing voc_read to return 'unsigned int' is better?


Didn't look at the rest (tired.)

(In reply to Ozkan Sezer from comment #1)
> 
> * load_voc.c:
> - sblen = ((bits24[0]) | (bits24[1] << 8) | (bits24[2] << 16));
> + sblen = (Uint32)((bits24[0]) | (bits24[1] << 8) | (bits24[2] << 16));
> 
> I _think_ it would be more correct doing like the following?
>   sblen = (Uint32)(bits24[0]) | ((Uint32)bits24[1] << 8) |
> ((Uint32)bits24[2] << 16);
> 
> Same for music_mad.c changes of the like.

I think, yes, until it will produce warnings on some compilers, otherwuse, give next:
```
sblen = (Uint32)((Uint32)(bits24[0]) | ((Uint32)bits24[1] << 8) |
> ((Uint32)bits24[2] << 16));
```

> 
> 
> * load_voc.c, voc_read():
> The procedure doesn't return negative. So, instead of casting done
> to (int), changing voc_read to return 'unsigned int' is better?
> 
> 
> Didn't look at the rest (tired.)

Yeah, if it never returns a negative value, it should return Uint32 as the "done" value without casting it into an int.

This has been added, thanks!
https://hg.libsdl.org/SDL_mixer/rev/9781cbae03a2

This introduced a build breakage, fixed here:
https://hg.libsdl.org/SDL_mixer/rev/5b7c09d1e108

This would have needed more reviewing / testing,
I guess.

I pushed some cleanups, still not fully happy but whatever.

About the newly added Mix_strtok_safe(): is it truly needed?
Does Mix_EachSoundFont() need reentrancy??

About the new MIX_UNUSED macro: it has multiple definitions
in many places. Is there a sane place to put it so that every
module can see it?

> https://hg.libsdl.org/SDL_mixer/rev/7846c39b6154
Whops! Yeah, this thing should be in part of this: https://bugzilla.libsdl.org/show_bug.cgi?id=3905

> About the newly added Mix_strtok_safe(): is it truly needed?
> Does Mix_EachSoundFont() need reentrancy??

I added Mix_strtok_safe() to remove ugliness and having to fell into unsafe "strtok()" which I want to avoid. Therefore I made Mix_strtok_safe(). Anyway, to be clear, I have suggested to add it into mainstream SDL as "SDL_strtok()" https://bugzilla.libsdl.org/show_bug.cgi?id=4046 to completely solve this (and other cases in other projects where "strtok()" is used).

> About the new MIX_UNUSED macro: it has multiple definitions in many places. Is there the same place to put it so that every module can see it?

You right, it should be in the united place, probably a new header, or one which is shared between most of the modules. The reason why I made multiple definitions because I didn't want to make new files in far past, which is... mistake, and I should add a small header to declare MIX_UNUSED() macro.

(In reply to Vitaly Novichkov from comment #6)
> [...] having to fell into unsafe "strtok()"

Asking again: Does Mix_EachSoundFont() need reentrancy?
Do we need safety of strtok_r() in there?

> https://bugzilla.libsdl.org/show_bug.cgi?id=4046

Added my note there.

> > About the new MIX_UNUSED macro: it has multiple definitions [...]
> 
> You right, it should be in the united place, probably a new header
> [...]

Or we can replace all instances of MIX_UNUSED(x) with plain
(void)x..  SDL itself doesn't have such a macro, why should
SDL_mixer have it.

> Or we can replace all instances of MIX_UNUSED(x) with plain
(void)x..  SDL itself doesn't have such a macro, why should
SDL_mixer have it.

I added the macro for convenience and nicer look (Inspired by Q_UNUSED() macro in Qt). Yeah, plain (void)x also should work. Yeah, you would introduce this macro into SDL as `SDL_UNUSED()` or something like, to use it for warning fixes.

> Asking again: Does Mix_EachSoundFont() need reentrancy? Do we need safety of strtok_r() in there?

There are applications that can dynamically change sound fonts on the fly (together with a song switching). Therefore, concurrent using of unsafe `strtok()` will lead to a crash. At the same time, an application may use strtok() by itself, and when it's run in multiple threads, you know the result.

(In reply to Vitaly Novichkov from comment #8)
> introduce this macro into SDL as `SDL_UNUSED()` or something like

SDL does have an SDL_UNUSED macro with a completely different
meaning.  I'm leaning on replacing MIX_UNUSED(x) with (void)x
if no objections.

Ozkan, one small note:

https://github.com/WohlSoft/SDL-Mixer-X/commit/37377d89743d5c71f13f5a5cfce0e48986797cfa

I removed this beautifying because of bunch of scarying GCC7+ warnings:

```
effect_position.c:1844: предупреждение: this ‘if’ clause does not guard... [-Wmisleading-indentation]
     if (left < 0) left = 0; if (left > 255) left = 255;
     ^~
```