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

Merged
SlavekB merged 1 commits from feat/fix-if-else-cond into master 3 months ago
obache commented 3 months ago

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

OSS MIDI API may be missing even if ALSA is enabled.
SlavekB reviewed 3 months ago
Please consider simplifying the code as suggested in the comment.
@@ -712,3 +713,4 @@
713
+#endif
712 714
 #ifdef HAVE_OSS_SUPPORT
713 715
     _seqbufptr=0;
714 716
 #endif
SlavekB

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

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

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 :smile_cat:

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 :smile_cat:
obache

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

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

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

For multiple sound systems support, is it intend to be runtime switching or build time?
SlavekB commented 3 months ago
Owner

Please give attention to my review – see comment above.

Please give attention to my review – see comment above.
obache commented 3 months ago
Poster

Changed to coding style same as others.

Changed to coding style same as others.
SlavekB deleted branch feat/fix-if-else-cond 3 months ago
SlavekB added this to the R14.0.6 release milestone 3 months ago
The pull request has been merged.
Sign in to join this conversation.
Loading…
Cancel
Save
There is no content yet.