[Info-vax] openvms and xterm
Dan Cross
cross at spitfire.i.gajendra.net
Sun May 5 19:07:43 EDT 2024
In article <v1487o$10cnq$1 at dont-email.me>,
Dave Froble <davef at tsoft-inc.com> wrote:
>On 5/3/2024 9:04 PM, Dan Cross wrote:
>> In article <v1366u$lf87$1 at dont-email.me>,
>> Dave Froble <davef at tsoft-inc.com> wrote:
>>> :-)
>>>
>>> The following is some snippets from what was basically a research program, and I
>>> consider commenting such should be rather terse. For production programs I'd
>>> want a lot more.
>
>Do consider the above ...
Sure. But I don't consider most of these "terse", though I do
wish that they had less boilerplate and more useful content.
That is, terse is fine if it's accompanied by high information
density. Verbose is strictly worse with lower information
density.
>> I hope you won't mind some feedback?
>
>No problem. But some background. I came from an environment where strict
>programming practices were in effect. Certain things, such as the header of a
>program file, had a strict format to follow, and much of the text came from an
>IDE code generator. That allowed any support person to know where to look for
>information, and what to look for.
>
> [snip]
>>> !********************************************************************
>>> !
>>
>> What does the line of asterisks buy you?
>
>Strict formatting in every program. Important information was always highlighted.
That's fair; I confess I have complex feelings around code style
guides that mandate things like that. On the one hand, I don't
think that this kind of visual artistry really adds all that
much, and can often be distracting; on the other, I worked in a
very large codebase with a rigid style guide that I disliked,
but which was a major factor in allowing that to scale to tens
of thousands of engineers working on ~2 BLOC (yes, "B") of code
simultaneously, all of which was mutually intelligible.
>>> ! Program: TCP_PEEK.BAS
>>
>> Why do you need this? Is it not in a file that is already
>> named, "TCP_PEEK.BAS"? What do you get by repeating it in the
>> file itself?
>
>Doesn't hurt.
...until you rename the file! Or copy and paste a part into a
different file, or similar maintenance activities. Then it adds
churn, unnecessary diff size, and cognitive overhead.
>>> ! Function: Test Using TCP/IP Sockets as a Listener
>>> ! Version: 1.00
>>> ! Created: 01-Dec-2011
>>> ! Author(s): DFE
>>
>> Why the elaborate formatting of this front matter? What does
>> some of it even mean? How did you decide that this was version
>> 1.00, for example? (Something like semver is much more useful
>> than an arbitrary major.minor here.) Is the creation date
>> useful versus, say, the last modification date?
>
>For some, the first iteration of a program is version 1. I guess other
>standards could be used.
It's a bit of an aside, but I do recommend checking out semver;
it's genuinely useful. The gist of it from https://semver.org/:
|Given a version number MAJOR.MINOR.PATCH, increment the:
|
|MAJOR version when you make incompatible API changes
|MINOR version when you add functionality in a backward compatible manner
|PATCH version when you make backward compatible bug fixes
>Both dates can be useful.
I'd argue that that's really better tracked in a revision
control system.
>> Moreover, where do you record the history of the module?
>> Knowing how code has evolved over time can be very useful.
>
>I didn't show that. As I wrote, some snippets from a program, not the entire
>program.
>
>> Frankly, all of this is metadata, which is better captured in a
>> revision control system than in comments at the top of a file,
>> which can easily get out of date over time.
>>
>>> !
>>> ! Purpose/description:
>>
>> Well, which is it?
>
>Now, that's a bit too "picky" ...
Maybe. But if one's going to offer up an example that shows the
kids how it oughta be done, don't be surprised if you get a
little bit of pushback. ;-)
I stand by the rest of my comments, quoted below. Honestly,
this code wouldn't pass review anywhere I've worked recently.
- Dan C.
Old message follows:
> !
> ! This program will set up TCP/IP sockets to allow
> ! itself to listen for connection requests. When
> ! a connection request is received, this program
> ! will accept the connection, and then attempt to
> ! PEEK the message, ie; read it but leave it available
> ! to be re-read.
This is good, but honestly, kind of all you need at the top of
the file.
> When using custom defined structures, it might be nice to know what they will be
> used for.
>
> !**************************************************
> ! Declare Variables of User Defined Structures
> !**************************************************
>
> DECLARE IOSB_STRUCT IOSB, ! I/O status blk &
> ITEMLIST_2 SERVER.ITEMLST, ! Server item list &
> ITEMLIST_2 SOCKOPT.ITEMLST, ! Socket options list &
> ITEMLIST_2 REUSEADR.ITEMLST, ! Reuse adr list &
> ITEMLIST_3 CLIENT.ITEMLST, ! Client item list &
> SOCKET_OPTIONS LISTEN.OPTN, ! Socket options &
> SOCK_ADDR CLIENT.ADR, ! Client IP adr/port &
> SOCK_ADDR SERVER.ADR, ! Server IP adr/port &
> BUFF CLIENT.NAME, ! Client name buffer &
> BUFF SERVER.NAME, ! Server name buffer &
> IP_ADR IP, ! Ip address &
> BUFF MSG ! Message buffer
I don't think that these comments are useful at all, with the
possible exception of the one on IOSB. These comments just
parrot the code. If I saw, "SOCKOPT.ITEMLIST" how is it not
obvious that that is a, "Socket options list"? Note also that
the comment omits the fact that it's an item list, which is a
specific thing, rather than just a generic list. Half of the
item lists are annotated as item lists in the comments, but
the other half are not.
> I consider the following rather terse. Either the programmer knows how to use
> system services, or perhaps remedial training is called for.
>
> !**************************************************
> ! Assign channels to 'TCPIP$DEVICE:'
> !**************************************************
>
> Dev$ = "TCPIP$DEVICE:"
>
> Stat% = SYS$ASSIGN( Dev$ , ListenCh% , , )
> If ( Stat% And SS$_NORMAL ) = 0%
> Then E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to assign listener channel - "; E$
> GoTo 4900
> End If
>
> Print #KB%, "Internal VMS channel for listener socket:"; ListenCh%
>
> Stat% = SYS$ASSIGN( Dev$ , ClientCh% , , )
> If ( Stat% And SS$_NORMAL ) = 0%
> Then E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to assign client channel - "; E$
> GoTo 4900
> End If
It's also rather repetitive. It'd be better to wrap assignment
in some kind of helper function, IMHO. Without knowing more
about VMS BASIC, however, it's difficult to tell whether one
could do much better.
> However, when details might be helpful, there is usually never too much.
>
> !**************************************************
> ! Create Listener socket
> ! Bind server's IP address and port # to listener
> ! socket, set socket as a passive socket
I'm not sure this comment is accurate: it appears that most of
what this code is doing until the $QIOW is setting up data for
the $QIOW. Something like this may be more useful:
! Initialize IOSB data for listener socket creation,
! then create the socket and bind it to the server's
! IP address with the given port number.
> ! Note: we used to do this in 2 calls, but can be combined
Grammar: "but they were combined." Again, we see how missing
the history can lead to questions: why were they combined, and
when? Is that better somehow, other than the general principle
of doing less work?
> !**************************************************
>
> LISTEN.OPTN::PROTOCOL% = TCPIP$C_TCP ! Listener socket optn
> LISTEN.OPTN::TYP$ = Chr$(TCPIP$C_STREAM)
> LISTEN.OPTN::AF$ = Chr$(TCPIP$C_AF_INET)
>
> SOCKOPT.ITEMLST::LEN% = 8% ! Socket options buffer
> SOCKOPT.ITEMLST::TYP% = TCPIP$C_SOCKOPT
> SOCKOPT.ITEMLST::ADR% = Loc(REUSEADR.ITEMLST::Len%)
>
> REUSEADR.ITEMLST::LEN% = 4% ! Reuse adr (port #)
> REUSEADR.ITEMLST::TYP% = TCPIP$C_REUSEADDR
> REUSEADR.ITEMLST::ADR% = Loc(ReuseAdrVal%)
>
> ReuseAdrVal% = 1% ! Set to 'True'
>
> SERVER.ITEMLST::LEN% = 16% ! Server item list
> SERVER.ITEMLST::TYP% = TCPIP$C_SOCK_NAME
> SERVER.ITEMLST::ADR% = Loc(SERVER.ADR::Fam%)
>
> SERVER.ADR::Fam% = TCPIP$C_AF_INET ! Server Ip adr/port
> SERVER.ADR::PORT% = SWAP%(ServerPort%)
> SERVER.ADR::IP.ADR% = TCPIP$C_INADDR_ANY
> SERVER.ADR::ZERO1% = 0%
> SERVER.ADR::ZERO2% = 0%
>
> BACKLOG% = 1%
>
> Stat% = SYS$QIOW( , ! Event flag &
> ListenCh% By Value, ! VMS channel &
> IO$_SETCHAR By Value, ! Operation &
> IOSB::Stat%, ! I/O status block &
> , ! AST routine &
> , ! AST parameter &
> LISTEN.OPTN::Protocol%, ! P1 &
> , ! P2 &
> SERVER.ITEMLST::Len%, ! P3 - local socket name &
> BACKLOG% By Value, ! P4 - connection backlog &
> SOCKOPT.ITEMLST::Len%, ! P5 - socket options &
> ) ! P6
>
> If ( Stat% And SS$_NORMAL ) = 0%
> Then E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to queue create and bind listener socket - "; E$
> GoTo 4900
> End If
>
> If ( IOSB::Stat% And SS$_NORMAL ) = 0%
> Then Stat% = IOSB::Stat%
> E$ = FnVMSerr$( Stat% )
> Print #KB%, "Unable to create and bind listener socket - "; E$
> GoTo 4900
> End If
>
> My opinion is, the above is essential, without it, there would be much studying
> of code, wondering what is being referenced, and such. I always use one line
> for each argument in a QIO and such, which makes it very clear what is
> happening. Without that, even the best will still have some "fun" reading the
> code to figure out what is happening.
I agree that well-commented code is useful.
More information about the Info-vax
mailing list