Re: more compile warnings with new factor

[Jim Meyering]

Pádraig Brady wrote:
> On 10/23/2012 08:46 AM, Jim Meyering wrote:
>> Torbjorn Granlund wrote:
>>> Pádraig Brady <address@hidden> writes:
...
>>> Aren't this Draconian options ("ask the compiler to nag about everything
>>> it can, then treat nagging as errors in the code") self-defeating?  You
>>> need to avoid natural ways of writing things, and instead go for
>>> contorted solutions.  I spent a few hours trying to satisfy your demands
>>> in this area, and I am sure you maintainers have spent a lot of time on
>>> it.
>>
>> We've learned that while many of the warnings are
>> not about real bugs, some do indicate real problems.
>> An unused macro or variable could well indicate a typo
>> in the definition/declaration or in all uses.
>>
>>> I for one prefer to spend my time on improving GNU, not define an
>>> artifical goal like this one, and work on satifying that.
>>
>> We have moved to using very strict warning settings gradually over the
>> years, carefully disabling the few warning options for which the cost
>> consistently outweighs the benefit.
>>
>> We really do believe that what we are doing is worthwhile.
>
> The onus is on those who know the code (now),
> to minimize warnings to ease maintenance down the line.

Precisely.  We are in the unusual position of being able to take
a very long view of software "health".  That puts a large weight
on long-term maintainability, sharing responsibility, documenting, etc.

In that spirit, and considering the little pang :-) I felt last night at
the dearth of comments in factor.c, we should add more comments there.
Each function should have a comment that describes what it does
in terms of its arguments.  To see a list of candidates, run this

    grep -A1 -B3 '^static' src/factor.c

Many of its macros deserve comments, too.

I'll start:
(just writing the comment for factor made me see
that I would prefer to have it take an algorithm-specifying
parameter rather than use that global variable.)

>From 225f340c367e2618b3e7d56c506893ea1c6e567f Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 23 Oct 2012 12:08:58 +0200
Subject: [PATCH] factor: add comments

* src/factor.c (is_square): Use active voice in comment, not passive.
(factor): Add function-describing comment.
(mp_factor): Likewise.
---
 src/factor.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/factor.c b/src/factor.c
index 73c59e9..e7d152b 100644
--- a/src/factor.c
+++ b/src/factor.c
@@ -1792,7 +1792,7 @@ isqrt2 (uintmax_t nh, uintmax_t nl)
 #define MAGIC65 ((uint64_t) 0x218a019866014613ULL)
 #define MAGIC11 0x23b

-/* Returns the square root if the input is a square, otherwise 0. */
+/* Return the square root if the input is a square, otherwise 0. */
 static uintmax_t _GL_ATTRIBUTE_CONST
 is_square (uintmax_t x)
 {
@@ -2167,6 +2167,8 @@ factor_using_squfof (uintmax_t n1, uintmax_t n0, struct 
factors *factors)
   return false;
 }

+/* Compute the prime factors of the 128-bit number (T1,T0), and put the
+   results in FACTORS.  Use the algorithm selected by the global ALG.  */
 static void
 factor (uintmax_t t1, uintmax_t t0, struct factors *factors)
 {
@@ -2197,6 +2199,8 @@ factor (uintmax_t t1, uintmax_t t0, struct factors 
*factors)
 }

 #if HAVE_GMP
+/* Use Pollard-rho to compute the prime factors of
+   arbitrary-precision T, and put the results in FACTORS.  */
 static void
 mp_factor (mpz_t t, struct mp_factors *factors)
 {
--
1.8.0