Re: This MIDI stuff, would someone be interested in reviewing my code?

[John McCabe <john@nospam.assen.demon.co.uk>]

On Mon, 08 Mar 2010 10:24:26 -0700, "Jeffrey R. Carter"
<spam.jrcarter.not@spam.acm.org> wrote:

>John McCabe wrote:
>> 
>> 1) Perhaps I should be using limited withs in some places to get
>> access to the primitive operators/functions of the stuff in
>> Interfaces.C/.strings and Win32 etc.
>
>That's not what "limited with" is for.

>I notice that you "use" a lot of pkgs, but then refer to things using full 
>names. You should get rid of those "use" clauses, and add "use type" clauses in 
>your declarative part as needed.

Ah - thanks. I was getting confused there between limited with and use
type. You've clarified that for me.

>> 3) Would it be more appropriate to use something like
>> Win32.UINT'Image() instead of getting an instantiation of the
>> Modular_IO package?

>That's a matter of taste and needed functionality.

>My first comments would be to use common Ada naming conventions: Dev_ID, 
>Num_Input_Devices, and so on.

Ok - I'll see what I can do.

>I also prefer to see "loop" and "then" on the same line as "for" and "if". This 
>looks like a C-family person thinking they're equivalent to '{'.

I was thinking of them being more equivalent to Ada's begin for loops.
I like the idea of having a visual clue, be it a begin, loop, then, {
or whatever on a line on its own to show clearly that the following
code is indented because it's within that construct.

>I would make Numinputdevices and Numoutputdevices constants.

They're not constants though - they're discovered from the
midiInGetNumDevs and midiOutGetnumDevs functions.

>For this small 
>example, I would do the same with Midiincaps and Midioutcaps; there's really no 
>need to free them, since they're around for the entire program, and will be 
>freed when the program exits.

The point of allocating/freeing them was to get round the fact that,
when they were declared in the declarative part of the main function,
I seemed to have to use 'Unchecked_Access when passing them to
midiInGetDevCaps/midiOutGetDevCaps.

>> with Interfaces.C; use Interfaces.C;
>> with Interfaces.C.Strings; use Interfaces.C.Strings;
>
>"with Interfaces.C.Strings;" implies "with Interfaces; with Interfaces.C;", so 
>there's no reason to have both.

Ok - thanks.

>> with Win32; use Win32;
>> with Win32.Mmsystem; use Win32.Mmsystem;

>Ditto.

Ditto too :-)

>>       for devId in Win32.UINT range 0..(NumInputDevices - 1)
>>       loop
>
>These parentheses are unnecessary.

Yes, I agree. I like to make things obvious though :-)

>>          res := Win32.Mmsystem.midiInGetDevCaps(devId,
>>                                                 midiInCaps,
>> 
>> (Win32.Mmsystem.MIDIINCAPS'size * Win32.BYTE'size));
>
>So are the internal ones here.

True.

>You've already been warned about sprinkling OS-dependent stuff throughout your code.

Yip.

Thanks for those comments. Much appreciated.