Guillaume -- sorry to be a bother when you've just applied this, but I evidently hadn't reviewed it well before. Any chance you could revert?
At a quick glance I can see at least two places in which this patch introduces bugs, and none in which it removes them. This is pretty sensitive code in places -- "just fixing compiler warnings" is not always as unintrusive as it seems. That doesn't mean the warnings wouldn't be better fixed, just that the fixes may require a bit more context. Details: On Sunday 15 May 2005 10:44, Guillaume Laurent wrote: > [...] > Modified Files: > AlsaDriver.cpp AudioProcess.cpp LADSPAPluginInstance.cpp > PlayableAudioFile.cpp PlayableAudioFile.h SoundDriver.cpp > SoundDriver.h > Log Message: > Applied sound/ part of patch from Stephen Torri <[EMAIL PROTECTED]>. > > [...] > *** PlayableAudioFile.h 8 Feb 2005 14:57:55 -0000 1.12 > --- PlayableAudioFile.h 15 May 2005 09:44:48 -0000 1.13 > *************** > *** 185,189 **** > InstrumentId m_instrumentId; > > ! int m_targetChannels; > int m_targetSampleRate; > > --- 185,189 ---- > InstrumentId m_instrumentId; > > ! unsigned int m_targetChannels; > int m_targetSampleRate; m_targetChannels is initialised to -1, and compared <= 0 later. It therefore needs to be signed. (Much of the rest of the patch concerned changing things that compare against m_targetChannels from ints to unsigneds because of the change of m_targetChannels to unsigned. I don't agree that that should be done.) > *** AudioProcess.cpp 17 Feb 2005 16:10:15 -0000 1.74 > --- AudioProcess.cpp 15 May 2005 09:44:48 -0000 1.75 > *************** > *** 174,178 **** > #endif > > ! int rv = pthread_join(m_thread, 0); > > #ifdef DEBUG_THREAD_CREATE_DESTROY > --- 174,178 ---- > #endif > > ! // UNUSED - int rv = pthread_join(m_thread, 0); The return value may be unused, but we still need the call to pthread_join! > *** AlsaDriver.cpp 13 May 2005 22:43:23 -0000 1.367 > --- AlsaDriver.cpp 15 May 2005 09:44:48 -0000 1.368 > *************** > *** 4654,4658 **** > > unsigned int t_skew = snd_seq_queue_tempo_get_skew(q_ptr); > ! unsigned int t_base = snd_seq_queue_tempo_get_skew_base(q_ptr); > > #ifdef DEBUG_ALSA > --- 4654,4658 ---- > > unsigned int t_skew = snd_seq_queue_tempo_get_skew(q_ptr); > ! // UNUSED - unsigned int t_base = > snd_seq_queue_tempo_get_skew_base(q_ptr); This causes a compile failure if DEBUG_ALSA is set (when t_base is used for a debug printout). It could be moved inside the DEBUG_ALSA guard, but it shouldn't be removed. Also, the patch makes a handful of stylistic changes that I wouldn't object to in someone else's new code, but that I do object to when applied as a modification to my own. The main one is changing code of the form int someIntValue = int(someUnsignedValue); or int someIntValue = (int)someUnsignedValue; to int someIntValue = static_cast<int>(someUnsignedValue); I have nothing against static_cast, but I don't want to use it just to cast away an unsigned. Comparing ordinals to cardinals is not always a bug, and we often enough also want ordinals "plus error value" (like m_targetChannels above). There are also a couple of cases where the name of a parameter has been removed from a function declaration in a base class with an empty declaration body (intended to default to no activity if the subclass doesn't override it). In these cases the parameter name must at least be present in /* */, because, well, it's important. Chris ------------------------------------------------------- This SF.Net email is sponsored by Oracle Space Sweepstakes Want to be the first software developer in space? Enter now for the Oracle Space Sweepstakes! http://ads.osdn.com/?ad_id=7393&alloc_id=16281&op=click _______________________________________________ Rosegarden-devel mailing list [email protected] - use the link below to unsubscribe https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
