Hello, Squeezed Ones.
I have a tendency to compile with as many warnings turned on as I can manage - as long as I can avoid getting any warnings in my actual code without doing bogus work.
This means I turn off a lot of default warnings on Visual Studio - what’s with those guys, why do they warn about the incredibly common idiomint x = getSize();
if (x) {
/* ... */
} by default?!
But I’m here to talk about warnings that I have turned on that produce warnings in Juce that could plausibly be avoided!
There are three that concern me:
1. No previous prototype for…
I turned this on a few months ago after an annoying debugging session where it turned out I had code in the wrong namespace and so a call was resolving to a wrong function. It’s simple enough - enabling this requires that every function you have is either declared before it’s defined (i.e., in a header file), or is file-scope only (i.e. static, or in an anonymous namespace) - or you get a warning.
I was surprised how useful this was, and how all the changes it forced me to make were good ones. I found one (small) duplicate function and several functions that just weren’t being used at all this way, and clarified a lot of code.
Now I think this should be compulsory! My reasoning: any function that is only referenced in one .cpp file should definitely be scoped just to that file with static or an anonymous namespace - in order to avoid unnecessary collisions, but even more, as documentation to the reader that these are “local” functions that they really don’t need to learn about unless they’re going deep into that file.
Conversely, any function defined in a .cpp file that’s used in another .cpp file should be declared in a header file, and that header file should be included by the file defining the function (as it will help sometimes catch the error when you change the definition of the function but not the declaration…)
This warning occurs an awful lot in Juce and I think every time it’s because there are functions that are only used in one compilation unit but aren’t referenced out of that file. It’d be a matter of great triviality to fix those errors.
2. Implicit conversion shortens 64-bit value into a 32-bit value
3. Comparison between signed and unsigned integer expressions
Now we get to one of the annoying features of programming in a language that has several different types of integers that are mostly but not entirely compatible.
Much of the time, these warnings reflect a conversion that’s necessary and correct. However, some reasonable fraction of the time, it points to a problem that actually exists. And unfortunately, for problem 2, the errors are often suppressed by putting in a bulky static_cast() statement.
I have to say that I have both of these warnings turned on in my code, out of habit if nothing else, and it has worked out to improve my code quality. I simply ignore the Juce warnings, which luckily only appear in the compilation of Juce code and don’t spring from any Juce headers.
For the unsigned warning, unfortunately Juce uses this peculiar idiom:for (int i = numDestChannels; --i >= 0;) quite a bit, so the solution I use in my code, which is using unsigned int for array variables, is a little more involved for Juce. If I were maintaining Juce, I might not consider this change worth the bangs for the bucks.
For the 64-32 warning, well…
It seems to me that the 64-32 warning finds all sorts of minor quality defects in the existing code. For example, it finds dozens of cases where the float versions of clib functions are used in a calculation that’s otherwise carried in double precision… and it’s likely that fixing these would make Juce a little bit faster (because you avoid two casts each time) and make Juce’s math a little bit more accurate.
More important, I’d have to say that after reading through a lot of these, it’s no longer clear to me that PositionableAudioSource will really work if you run continuously for over std::numeric_limits::max() samples, which is about 13 1-2 hours at 44.1k.
I know for a fact that my program doesn’t work properly that way (again, a bug only I have detected :-D), and I’ve been pretty careful to do all my sample calculations in long long (what I call “int64”). Now, this is what I call a “low priority bug” for now, but for a later application where I’m expecting people to make very long recordings, it will be a serious issue.
I know this is quite a bit of effort but I’d be very much willing to assist in this work…