Re: [cp-patches] Patch: start of ALSA MIDI provider code

[Mark Wielaard]

Hi,

On Sun, 2005-10-02 at 01:01 -0700, Anthony Green wrote:
> Here's the start of ALSA MIDI provider code.  MIDI IN ports basically
> work.  You can read and print events from a MIDI keyboard using the
> standard interfaces.  It's not perfect, but it's a start.

Cool stuff.
You call the jni library "gjsmalsa", what does that stand for?

>  dnl -----------------------------------------------------------
> +dnl ALSA code (enabled by default)
> +dnl -----------------------------------------------------------
> +AC_ARG_ENABLE([alsa],
> +              [AS_HELP_STRING(--disable-alsa,compile ALSA providers 
> (disabled by --disable-alsa) [default=yes])],
> +              [case "${enableval}" in
> +                yes) COMPILE_ALSA=yes ;;
> +                no) COMPILE_ALSA=no ;;
> +                *) COMPILE_ALSA=yes ;;
> +              esac],
> +              [COMPILE_ALSA=yes])
> +if test "x$COMPILE_ALSA" = "xyes"; then
> +  AC_CHECK_HEADERS([alsa/asoundlib.h],,COMPILE_ALSA=no)
> +fi
> +AM_CONDITIONAL(CREATE_ALSA_LIBRARIES, test "x${COMPILE_ALSA}" = xyes)

I think it should say AS_HELP_STRING(--enable-alsa,... reading
--disable-alsa in the configure help output here is a but confusing.
Also when running configure with --disable-alsa I get:
        configure: error: conditional "AMDEP" was never defined.
I couldn't figure out why.

Feel free to call the new native directory just 'alsa' or 'midialsa'
instead of the super long gnu-javax-sound-midi-alsa. That might get
boring and we don't have that much jni code that we really need to use
package namespacing to disambiguate things.

In your .c files please use the copyright boilerplate as is. So don't
reindent it. It should be similar to how you did the .java file
boilerplate.

Instead of things like   /* Nothing yet.  */ please use FIXME to help
people find unimplemented things. Although in general you should just
not stub things but leave them out for now.

Please don't use abort() or printf() but do something like:
        
        JCL_ThrowException (env, "java/lang/InternalError",
                            snd_strerror(error));
        return;

The .java source files don't compile with gcj (4.0.2).
/usr/bin/gcj -Wno-deprecated --encoding=UTF-8 --bootclasspath '' --classpath 
..:../vm/reference:..:../external/w3c_dom:../external/sax:.: -C -d . -MD -MF 
lists/gnu-javax-sound.deps -MT lists/gnu-javax-sound.stamp -MP 
@lists/gnu-javax-sound.list
../gnu/javax/sound/midi/alsa/AlsaPortDevice.java:91: error: Type 'Info' not 
found in the declaration of the return type of method 'getDeviceInfo'.
     public Info getDeviceInfo()
            ^
It does compile with jikes though. So this might be a gcj or build
infrastructure bug.

There is lots and lots of stuff that look autogenerated:

  /* (non-Javadoc)
   * @see javax.sound.midi.MidiDevice#getTransmitter()
   */
  public Transmitter getTransmitter() throws MidiUnavailableException
  {
    // TODO Auto-generated method stub
    return null;
  }

Please don't include that (non-Javadoc) comment.
And are you sure you really need all this stuff already?
I really don't want to have so many stubs in our code. So if at all
possible just leave things out that are not necessary yet.

And please try to keep your lines < 80 characters for readability.

Cheers,

Mark

signature.asc
Description: This is a digitally signed message part