On Wed, Jan 28, 2015 at 04:59:13PM -0700, Alexej Magura wrote:
The patch attached adds a binding to /strerror /to
posix-common.scm^* (version 4.9.0.1). The other attachment is the
patched file, to make it easier to merge the changes with upstream.
I've already tested the changes: it compiles and works, at least in
/csi/.
^* /strerror/ is specified by both C89 and C99, so I figure that
it's probably available on both *nix and Windows systems, hence
adding it in posix-common.scm instead of posixunix.scm or some other
file.
Hello Alexej,
First off, thanks for your effort. We always appreciate contributions
from new people!
What I'm missing from your mail is a motivation as to why this needs
to be in CHICKEN core; it's trivially added to user code, and the cases
in which this is useful are (AFAIK) rather limited: As far as I can tell,
this would be useful only if you're calling some other C functions
directly in your own code and extracting errno. Besides, there's
already "posix-error" which does much the same (but also signals an
error condition, which is usually what you'd want if you're already
extracting the human-readable error string).
Furthermore a few more comments about the patch itself:
- Your patch seems to modify several lines with no change(?). Perhaps
this is an automatic whitespace cleanup from your editor or something,
but that means the patch isn't exactly "clean": the ideal patch only
changes what's necessary (and perhaps cleans up only immediately
surrounding code). This makes it easier to apply to different
branches or after several changes have been made, for example.
- Sending the complete new file is unnecessary. We can't use it except
for generating the same patch, anyway.
- It's better to send the output of "git format-patch"; this will
automatically credit the commit to you in the git history, and
allows you to determine the best commit message to accompany this
change in the history.
- You didn't add this new procedure to the manual.
- You didn't add the new procedure to posix.import.scm, so it wont't
be available to code that's in a module.
Please don't let my comments discourage you from sending more patches,
I'm giving this feedback to help you send better patches in the future!
Cheers,
Peter