unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
From: Thien-Thi Nguyen <ttn@gnuvola.org>
To: ludo@gnu.org (Ludovic Courtès)
Cc: guile-devel@gnu.org
Subject: Re: [Guile-commits] GNU Guile branch, master, updated. release_1-9-11-73-gedb3cfc
Date: Wed, 16 Jun 2010 00:22:45 +0200	[thread overview]
Message-ID: <877hm0rl56.fsf@ambire.localdomain> (raw)
In-Reply-To: <87pqzsj970.fsf@gnu.org> ("Ludovic Courtès"'s message of "Tue, 15 Jun 2010 23:08:03 +0200")

() ludo@gnu.org (Ludovic Courtès)
() Tue, 15 Jun 2010 23:08:03 +0200

   This file is from Gnulib.  So get the patch accepted in Gnulib and it
   will get in next time we update our Gnulib copy.

Done (commit e0fcde8130473202a4dd37c41a3c331fc5a9907d)!

   OK.  How about moving the AC_DEFUN in ‘acinclude.m4’?

Sure.

   Also, the comment above should say what the macro does, rather than how
   it could possibly do it in the future.

I'd say both kinds of info have their place in the comments.  In this
particular case, the macro is so simple, i did not think to include a
proper description (plus, the definition is immediately prior to its use).
However, moving its definition away from its use is a good time to add such.

   The ‘--ttn’ can also be omitted since we’d rather communicate by email
   than via comments in ‘configure.ac’.  :-)

That's a personal style point, i'd say.  Do you really insist on its removal?

   > +@deffn {Scheme Procedure} tmpfile
   > [...]

   I suggest adding this:

     (@pxref{Temporary Files,,, libc, The GNU C Library Reference Manual}).

Yes, good idea.

   > always returns the symbol @code{tmpfile}.

   What about returning #f instead?  That would seem more consistent since
   currently ‘port-filename’ can only return either #f or a string AFAIK.

I spewed a bit on this in my reply to Andy.  Gist: meh.

   > +  FILE *rv;
   > +
   > +  if (! (rv = tmpfile ()))

   Rather like this (info "(standards) Syntactic Conventions"):

     rv = tmpfile ();
     if (rv == NULL)
       ...

OK.

   Modulo these comments, feel free to commit.  If you’re now confident
   with rebase et al., you can commit to ‘master’.  Otherwise I can pick
   them later from your branch and push them to ‘master’.

Thanks for the review (even the stuff i hadn't meant to push).  I'll stick
with commiting to ttn/* for the time being, until i get more practice.

thi



  reply	other threads:[~2010-06-15 22:22 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <E1OOUGG-0001JC-VC@vcs-noshell.in.savannah.gnu.org>
2010-06-15 21:08 ` [Guile-commits] GNU Guile branch, master, updated. release_1-9-11-73-gedb3cfc Ludovic Courtès
2010-06-15 22:22   ` Thien-Thi Nguyen [this message]
2010-06-15 22:35     ` Ludovic Courtès
2010-06-16  7:25       ` Andy Wingo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/guile/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877hm0rl56.fsf@ambire.localdomain \
    --to=ttn@gnuvola.org \
    --cc=guile-devel@gnu.org \
    --cc=ludo@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).