Re: [PATCH] Added srfi-214: flexvectors

["Ludovic Courtès" <ludo@gnu.org>]

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