Hi Ludovic,

Thanks for the quick reviews.  I pushed the first two patches, after
incorporating your suggestions.

ludo@gnu.org (Ludovic Courtès) writes:

> Looks good.
>
> Minor comments:
>
>> +@deffn {Scheme Procedure} open-file filename mode @
>> +                          [#:guess-encoding=#f] [#:encoding=#f]
>> +@deffnx {C Function} scm_open_file_with_encoding @
>> +                     (filename, mode, guess_encoding, encoding)
>>  @deffnx {C Function} scm_open_file (filename, mode)
>>  Open the file whose name is @var{filename}, and return a port
>>  representing that file.  The attributes of the port are
>> @@ -900,8 +903,17 @@ to the underlying @code{open} call.  Still, the flag is generally useful
>>  because of its port encoding ramifications.
>>  @end table
>>  
>> -If a file cannot be opened with the access
>> -requested, @code{open-file} throws an exception.
>> +Unless binary mode is requested, the character encoding of the new port
>> +is determined as follows: First, if @var{guess-encoding} is true,
>> +heuristics will be used to guess the encoding of the file.  If it is
>
> “heuristics” is vague.  I’d prefer “the ‘file-encoding’ procedure is
> called to check for Emacs-style coding declarations (@pxref{Character
> Encoding of Source Files})”.  Should BOMs also be mentioned?

Makes sense.  Following our discussion on IRC, the patch below has the
following wording:

  Unless binary mode is requested, the character encoding of the new
  port is determined as follows: First, if @var{guess-encoding} is true,
  the @code{file-encoding} procedure is used to guess the encoding of
  the file (@pxref{Character Encoding of Source Files}).  [...]

I left out the "Emacs-style coding declarations" language here, because
I want to leave open the possibility of adding additional heuristics to
'file-encoding' in the future.

Furthermore, I've attached an additional patch below that changes the
"Character Encoding of Source Files" node to make that more clear.
Please let me know what you think.

>> +SCM k_guess_encoding = SCM_UNDEFINED;
>> +SCM k_encoding       = SCM_UNDEFINED;
>
> Add ‘static’.

Gah, thanks for catching that.  Oops!

>> +  scm_c_bind_keyword_arguments (FUNC_NAME, keyword_args, 0,
>> +                                k_guess_encoding, &guess_encoding,
>> +                                k_encoding, &encoding,
>> +                                SCM_UNDEFINED);
>
> Comes in handy.  ;-)

This patch was the motivation for adding that :)

>> +(define* (open-input-file
>> +          str #:key (binary #f) (encoding #f) (guess-encoding #f))
>> +  "Takes a string naming an existing file and returns an input port
>> +capable of delivering characters from the file.  If the file
>> +cannot be opened, an error is signalled."
>
> It’s a detail, for these procedures, I would s/str/file/, and in
> docstrings s/file STR/FILE/.

Good idea.

An updated patch, and the aforementioned new patch for the
'file-encoding' docs, follow.  More thoughts?

    Thanks!
      Mark