Re: [Rosegarden-bugs] sound AlsaDriver.cpp AudioProcess.cpp LADSPAPluginInstance.cpp PlayableAudioFile.cpp PlayableAudioFile.h SoundDriver.cpp SoundDriver.h

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Rosegarden-bugs] sound AlsaDriver.cpp AudioProcess.cpp LADSPAPluginInstance.cpp PlayableAudioFile.cpp PlayableAudioFile.h SoundDriver.cpp SoundDriver.h

Chris Cannam

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 <[hidden email]>.
>
> [...]
> *** 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
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
Reply | Threaded
Open this post in threaded view
|

Re: Re: [Rosegarden-bugs] sound AlsaDriver.cpp AudioProcess.cpp LADSPAPluginInstance.cpp PlayableAudioFile.cpp PlayableAudioFile.h SoundDriver.cpp SoundDriver.h

Guillaume Laurent
On Sunday 15 May 2005 12:09, Chris Cannam wrote:
> 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?

*sigh*. Sorry, I thought this part was ok with you, I'll revert it.

> 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.

Yes, I already had underlined these, and I thought there was no such changes
in sound/. Seems I didn't look well enough.

--
Guillaume.
http://www.telegraph-road.org


-------------------------------------------------------
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
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
Reply | Threaded
Open this post in threaded view
|

Re: Re: [Rosegarden-bugs] sound AlsaDriver.cpp AudioProcess.cpp LADSPAPluginInstance.cpp PlayableAudioFile.cpp PlayableAudioFile.h SoundDriver.cpp SoundDriver.h

Chris Cannam
On Sunday 15 May 2005 11:18, Guillaume Laurent wrote:
> On Sunday 15 May 2005 12:09, Chris Cannam wrote:
> > 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?
>
> *sigh*. Sorry, I thought this part was ok with you, I'll revert it.

Thanks.  I think I just hadn't read through the patch that far.  The
stuff in base/ looks fine, and I don't remember any problems in the
C++-only bits in gui/.

Funny really -- avoiding compiler warnings can be a surprisingly tricky
thing to do.  I had a phase of doing that a while ago (when we briefly
toyed with doing daily builds with -Werror) and I won't object to
others trying it, but at the time I reckoned there were always going to
be diminishing returns for fixing the last few warnings -- too easy to
break something, too hard to fix correctly, not necessarily any real
bug in the first place.


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
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
Reply | Threaded
Open this post in threaded view
|

Re: Re: [Rosegarden-bugs] sound AlsaDriver.cpp AudioProcess.cpp LADSPAPluginInstance.cpp PlayableAudioFile.cpp PlayableAudioFile.h SoundDriver.cpp SoundDriver.h

Guillaume Laurent
On Sunday 15 May 2005 20:46, Chris Cannam wrote:
>
> Thanks.  I think I just hadn't read through the patch that far.  The
> stuff in base/ looks fine,

That part is still in.

> and I don't remember any problems in the
> C++-only bits in gui/.

Except that 80% of it breaks KDE 3.1 compatibility, and I don't feel much like
weeding out what's useful. And there's also the groundwork for Stephen's
guitar chord editor (basically a couple of slots and an action), which don't
really belong in cvs yet.

--
Guillaume.
http://www.telegraph-road.org


-------------------------------------------------------
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
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel