Fix if-else syntax for the case missing OSS support #22

Merged

SlavekB merged 1 commits from feat/fix-if-else-cond into master

obache commented

Collaborator

OSS MIDI API may be missing even if ALSA is enabled.

SlavekB reviewed

SlavekB left a comment

Owner

Please consider simplifying the code as suggested in the comment.

libtdemid/deviceman.cc

													
					#endif

					#ifdef HAVE_OSS_SUPPORT

					    _seqbufptr=0;

					#endif

SlavekB commented

Owner

The combination of nested #ifdef and if-else in the code looks hard to read. I propose simplification as follows:

#ifdef HAVE_ALSA_SUPPORT
  if (alsa)
  {
    ((AlsaOut *)device[default_dev])->seqbuf_clean();
    return;
  }
#endif
#ifdef HAVE_OSS_SUPPORT
  {
    _seqbufptr=0;
   return;
  }
#endif

MicheleC commented

Owner

a simpler version is:

#ifdef HAVE_ALSA_SUPPORT
  if (alsa)
  {
    ((AlsaOut *)device[default_dev])->seqbuf_clean();
    return;
  }
#endif
#ifdef HAVE_OSS_SUPPORT
  _seqbufptr=0;
#endif

SlavekB commented

Owner

Yes, of course, this can make the code even simpler. I chose the closed block also for OSS to make it clear if there will be added support for some other sound systems – for example, for Solaris 😸

obache commented

Poster

Collaborator

For multiple sound systems support, is it intend to be runtime switching or build time?

obache commented

Poster

Collaborator

For multiple sound systems support, is it intend to be runtime switching or build time?

SlavekB commented

Owner

Please give attention to my review – see comment above.

obache commented

Poster

Collaborator

Changed to coding style same as others.

SlavekB closed this pull request

SlavekB deleted branch feat/fix-if-else-cond

SlavekB added this to the R14.0.6 release milestone

The pull request has been merged as dd6da7345a.

Sign in to join this conversation.

No reviewers

No Assignees

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdelibs#22