emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: libnettle/libhogweed WIP


From: Eli Zaretskii
Subject: Re: libnettle/libhogweed WIP
Date: Sat, 03 Jun 2017 10:23:39 +0300

> From: Ted Zlatanov <address@hidden>
> Date: Wed, 31 May 2017 14:17:54 -0400
> 
> I'd love to merge this branch, if there are no objections or comments on
> the two items above or otherwise. It's been sitting for a while.

I think it's okay to merge this, after fixing the following minor
issues:

There are a few TODOs in the documentation and in the code.  The one
in the docs should be either removed or moved to a comment so that it
doesn't appear in the manual.  Those in the code should be reviewed,
and in each case please decide whether the TODO will be handled any
time soon, or maybe should be simply deleted.

This fragment from the docs:

  +The functions described here accept argument lists of the
  +form @code{(BUFFER-OR-STRING START END CODING-SYSTEM NOERROR)},
  +where BUFFER-OR-STRING ...
  +
  +can be a string or a buffer or a list in the format
  address@hidden(BUFFER-OR-STRING START END CODING-SYSTEM NOERROR)} where only
  +the first element is required.

should have @w{..} around the long form, to prevent it from being
broken between 2 lines.  I also don't understand the ellipsis which
breaks the sentence in two -- I think it should simply be deleted.  As
for using @var, you should do it for every element individually, and
the elements should not be up-cased.  Likewise in other uses of @var.

In several cases in the docs (manual and doc strings) and in error
messages you have only one space between sentences; please use two
everywhere.

This text:

  address@hidden (@code{iv-auto} @var{length})
  +This will generate an IV of the specified length using the GnuTLS
  +GNUTLS_RND_NONCE generator.

uses "IV" and "iv-auto" without explaining what that is.  Other parts
have the same problem.

In this fragment:

  +The @var{input} can be specified as a buffer or string or in other
  +ways (@xref{Format of GnuTLS Cryptography Inputs}).

please use @pxref, as @xref is not appropriate for parenthesized
reference.  There are several instances of this in the docs changes.

Please make sure that "nil" is always in @code in the docs.

  +  Lisp_Object object        = Fnth (make_number (0), spec);
  +  Lisp_Object start        = Fnth (make_number (1), spec);
  +  Lisp_Object end          = Fnth (make_number (2), spec);
  +  Lisp_Object coding_system = Fnth (make_number (3), spec);
  +  Lisp_Object noerror      = Fnth (make_number (4), spec);

Isn't it simpler to use XCAR and XCDR here?  You are extracting
all the elements of a list in strict order, so Fnth is an overkill,
and will be slower.

  +      if (! INTEGERP (start))
  +        {
  +          error ("Without a length, 'iv-auto can't be used. See manual.");
  +          object = Qnil;

I think the quoting of iv-auto here is incorrect.  Likewise in other
similar error messages

  +  void *(*hash_func) (const char *, size_t, void *);

The leftmost '*' is unnecessary, I think.

  +      Lisp_Object cp = listn (CONSTYPE_HEAP, 15,
  +                              // A symbol representing the cipher
  +                              intern (gnutls_cipher_get_name (gca)),
  +                              // The internally meaningful cipher ID

I think we still prefer the /* .. */ style of comments.

  +  if (ret < GNUTLS_E_SUCCESS)
  +    {
  +      const char* str = gnutls_strerror (ret);
  +      if (!str)
  +        str = "unknown";
  +      error ("GnuTLS AEAD cipher %s/%s initialization failed: %s",
  +             gnutls_cipher_get_name (gca), desc, str);
  +      return Qnil;
  +    }

You don't need a 'return' after calling 'error' (here and elsewhere),
as the latter doesn't return.

Some of the lines in doc strings you wrote are too long, please make
sure they are no longer than 76 characters, and in any case fit on a
single 80-column line.

Also, the first line of a doc string should be a complete sentence.
This one, for example, isn't:

  +DEFUN ("gnutls-digests", Fgnutls_digests, Sgnutls_digests, 0, 0, 0,
  +       doc: /* Return alist of GnuTLS digest-algorithm method
  +descriptions as plists.  Use the value of the alist (extract it with
  +`alist-get' for instance) with `gnutls-hash-digest'.  The alist key is
  +the digest-algorithm method name. */)

  +  if (STRINGP (hash_method))
  +    {
  +      hash_method = intern (SSDATA (hash_method));
  +    }

No need for braces when there's only 1 line in the block.

Thanks again for working on this.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]