| Message ID | cover.1709580311.git.takev@disroot.org |
|---|---|
| Headers | show |
| Series | Add cyclone scheme | expand |
Hello, Thanks for this patch! It's always good to get more Scheme into Guix :) From a purely technical perspective, there's nothing wrong with these patches. They build fine, are reproducible, and seem well-enough-styled for all practical purposes. Make sure to run `guix lint` over the patches; I get a couple warnings when I do that locally. As a sidenote, Cyclone fails to build on a supported architecture (aarch64) because the `ck` package fails to build there. Interestingly, it builds fine on 32-bit ARM (armhf). I note this merely for posterity and anyone interested in fixing `ck`. Everything builds just fine on x86(_64). What follows is largely stylistic feedback. First patch: > ;; cyclone is self-hosted. To build it, we require the bootstrapped compiler. I would capitalize "Cyclone" here and elsewhere; it seems to be capitalized in the project's prose about it. Also, this is the "bootstrap" compiler -- the "bootstrapped" compiler would be the compiler compiled with this one. > (synopsis "Install Cyclone Scheme on your machine.") This is inaccurate as this package does not install Cyclone Scheme on the user's machine. Perhaps simply "Cyclone Scheme bootstrap compiler" or "Bootstrap Cyclone Scheme" would work? Synopses are, notably, not meant to be complete sentences and thus should not have final punctuation. > (description "Bootstrap the generate the cyclone scheme compiler") This description is a bit short. I would just combine the first two paragraphs of the project README into one with minor edits to get the following description string: "Cyclone Scheme is a brand-new, R7RS Scheme-to-C compiler that uses a variant of Cheney on the MTA to implement full tail recursion, continuations, and generational garbage collection. This package uses intermediate code generated by compiling the Scheme source files to build and install Cyclone Scheme. The compiler is self-hosting and cannot be built directly on a system without Cyclone binaries installed." Strictly speaking, "brand new" does not need a hyphen, but that's the way the project writes it so I've left it as-is. You probably also want to add a linebreak after `description` so the string starts on its own line; imo this makes the line alignment more esthetically pleasing when complying with the 80-column width limit. Second patch: > ;; the bootstrapped compiler and final compiler share most build reqs "bootstrapped" -> "bootstrap" > (synopsis "Fast R7RS scheme which compiles to C") Capitalize "Scheme." This could also just be "Fast R7RS Scheme-to-C compiler" if you want. > (description > "A brand-new compiler that allows practical application > development using R7RS Scheme. We provide modern features and a stable > system capable of generating fast native binaries.") I would restore the README's full "Cyclone Scheme is a" at the beginning of this; the description can refer to the package in the third person. Also, first-person language should probably be avoided here. "We" in a Guix package description would most directly imply the Guix project; this is inaccurate. Perhaps replace "We provide" with "Cyclone provides." It may also be fruitful to cut the second sentence entirely and enumerate some of the compiler's said features. I would try to modify the language of the second paragraph in the project's README (replacing passive voice with active voice, for example), and/or add a list highlighting some (all would be a bit too long I think) of the "Features" section of the aforementioned README. They seem very proud of the "Cheney on the MTA" algorithm so make sure to highlight it. Other than those stylistic notes, this patch series looks good to me! Thanks, Juli
LGTM!