comp.lang.ada
 help / color / mirror / Atom feed
From: "Jeffrey R. Carter" <spam.jrcarter.not@spam.acm.org>
Subject: Re: This MIDI stuff, would someone be interested in reviewing my code?
Date: Mon, 08 Mar 2010 10:24:26 -0700
Date: 2010-03-08T10:24:26-07:00	[thread overview]
Message-ID: <hn3c2q$4vk$1@tornado.tornevall.net> (raw)
In-Reply-To: <uun9p5dv8raemp9rrrbmfl08i8fqhjgqe9@4ax.com>

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.

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

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 would make Numinputdevices and Numoutputdevices constants. 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.

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

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

Ditto.

>       for devId in Win32.UINT range 0..(NumInputDevices - 1)
>       loop

These parentheses are unnecessary.

>          res := Win32.Mmsystem.midiInGetDevCaps(devId,
>                                                 midiInCaps,
> 
> (Win32.Mmsystem.MIDIINCAPS'size * Win32.BYTE'size));

So are the internal ones here.

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

-- 
Jeff Carter
"C's solution to this [variable-sized array parameters] has real
problems, and people who are complaining about safety definitely
have a point."
Dennis Ritchie
25



  parent reply	other threads:[~2010-03-08 17:24 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-03-08 11:40 This MIDI stuff, would someone be interested in reviewing my code? John McCabe
2010-03-08 11:52 ` Gautier write-only
2010-03-08 12:30   ` John McCabe
2010-03-08 17:24 ` Jeffrey R. Carter [this message]
2010-03-09 10:21   ` John McCabe
2010-03-09 12:10     ` Brian Drummond
2010-03-09 12:26       ` John McCabe
2010-03-09 12:42         ` John McCabe
2010-03-10 11:12           ` Stephen Leake
2010-03-10 12:29             ` John McCabe
2010-03-09 12:24     ` John McCabe
2010-03-09 13:11       ` Martin
2010-03-09 14:00         ` John McCabe
2010-03-13  8:12 ` Christophe Chaumet
2010-03-15 11:35   ` John McCabe
replies disabled

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox