Re: [PATCH] Added srfi-214: flexvectors

[Ludovic Courtès]

The following message is a courtesy copy of an article
that has been posted to gmane.lisp.guile.devel as well.

Hi Vijay,

Sorry for the looong delay!

Overall the patches look great to me.  Below are comments and
suggestions, mostly cosmetic.

Vijay Marupudi <vijay@vijaymarupudi.com> skribis:

> From 42206dec4d5e9ae51665c6e98ef07715b89b12fe Mon Sep 17 00:00:00 2001
> From: Vijay Marupudi <vijay@vijaymarupudi.com>
> Date: Tue, 18 Jan 2022 20:52:08 -0500
> Subject: [PATCH] Added srfi-214: flexvectors
>
> Included code, documentation, and tests

Please use the ChangeLog style for commit messages (see ‘git log’ for
examples); as a welcome gift, we can do it for you though.  :-)


[...]

> +@node Flexvectors
> +@subsection Flexvectors
> +@cindex flexvector

General comments:

  • please leave two spaces after end-of-sentence periods (a convention
    eases navigation with Emacs);

  • use @dfn to decorate a term the first time it’s introduced, @code
    for identifiers, etc.;

  • limit lines to ~80 columns.

> +Flexvectors are sometimes better known as a 
> @url{https://en.wikipedia.org/wiki/Dynamic_array,dynamic arrays}. This data
> +structure has a wide variety of names in different languages:

Maybe add a first sentence before this one, like: “The @code{(srfi
srfi-214)} module implements @dfn{flexvectors}, a data structure for
mutable vectors with adjustable size.”

> +@itemize @bullet{}

You can drop @bullet{}.

> +@item
> +JavaScript and Ruby call it an array
> +@item
> +Python calls it a list
> +@item
> +Java calls it an ArrayList (and, before that, it was called a Vector)

Add a semicolon at the end of the first two lines, a period for the last
one.  @code{ArrayList}.


[...]

> +++ b/module/srfi/srfi-214.scm
> @@ -0,0 +1,735 @@
> +;;; -*- mode: scheme; coding: utf-8; -*-
> +;;;
> +;;; Copyright (C) Adam Nelson (2020).
> +;;; Copyright (C) Vijay Marupudi (2022).

Nitpick: should be:

  Copyright (C) 2022 …

> +(define-module (srfi srfi-214))
> +
> +(use-modules ((scheme base)
> +              #:prefix r7:)
> +             (scheme case-lambda)
> +             (scheme write)
> +             (srfi srfi-1)
> +             (srfi srfi-9)
> +             (srfi srfi-9 gnu)
> +             (srfi srfi-11)
> +             (rnrs io ports))
> +
> +
> +(export ; Constructors

Please use a single ‘define-module’ clause with #:use-module and
#:export lines (this is equivalent but that’s what we conventionally
use and it makes the interface clearer upfront).

Avoid using R6RS and R7RS modules as they pull in a whole lot of stuff.
For example, maybe you can use (ice-9 binary-ports) instead of (rnrs io
ports); maybe you don’t need (scheme …) at all because Guile most likely
has equivalent things built in.

> +        make-flexvector flexvector
> +        flexvector-unfold flexvector-unfold-right
> +        flexvector-copy flexvector-reverse-copy
> +        flexvector-append flexvector-concatenate flexvector-append-subvectors
> +        <flexvector>

Don’t export <flexvector> as it would expose implementation details (the
record interface).

> +(define (flexvector . xs)

In general we try to add docstrings for all public top-level
procedures.  Bonus points if you can do that.  :-)

The rest looks great to me!

Could you send an updated patch?

Thanks,
Ludo’.