timevar: small tweaks

timevar: small tweaks

[Bruno Haible]

Hi Akim,

How about the following patches:

* In timevar.c: Include timevar.h first. This is a Gnulib best practice, which
  has the benefit of verifying that the header file is self-contained.

* In timevar.def:
  - The DEFTIMEVAR invocation must NOT be followed by a semicolon, otherwise
    you'll get a syntax error in the enum definition.
  - Enum values are not positive integers. The first enum value is 0.
  - What is the difference between a "character string" and a "string"?
  - The statement that the timing table is printed in the given order
    contradicts the comment in timevar.h.

* In timevar.h:
  - Better talk about the program or the application, not the "compiler".
  - It is pointless to omit the parameter names from the function declarations
    if the comments talk about TIMEVAR, ELAPSED, FP, etc.

* In the module description:
  Are you subscribed to bug-gnulib? If not, you should be listed as the
  maintainer of the modules, so that people CC you when they have questions
  or patches about it.


diff --git a/lib/timevar.c b/lib/timevar.c
index 0578816..a2d2433 100644
--- a/lib/timevar.c
+++ b/lib/timevar.c
@@ -20,6 +20,9 @@
 
 #include <config.h>
 
+/* Specification.  */
+#include "timevar.h"
+
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -101,8 +104,6 @@ static float clocks_to_msec;
 # define CLOCKS_TO_MSEC (1.0 / CLOCKS_PER_SEC)
 #endif
 
-#include "timevar.h"
-
 /* See timevar.h for an explanation of timing variables.  */
 
 int timevar_enabled = 0;
diff --git a/lib/timevar.def b/lib/timevar.def
index e5cc3ff..d640b30 100644
--- a/lib/timevar.def
+++ b/lib/timevar.def
@@ -24,21 +24,21 @@
 
    Syntax:
 
-     DEFTIMEVAR (Id, Name);
+     DEFTIMEVAR (Id, Name)
 
    where:
 
    - Id is the value used to identify the timing variable.
-     It's an enum value, i.e., behaves like a positive integer.
+     It's an enum value, i.e., behaves like a nonnegative integer.
      It is used only the manipulate the timer: the user of the
      program will never see this identifier.
 
-   - Name is a character string describing its purpose.  This string
+   - Name is a string describing its purpose.  This string
      will be presented to the user in the timing tables.  It does not
      need to be a literal, you may for instance use gettext.
 
    The order of this list matters: that is the order in which the
-   timing table is printed.
+   timing table is printed, except that 'tv_total' is printed last.
   */
 
 /* The total execution time.  Mandatory.  */
diff --git a/lib/timevar.h b/lib/timevar.h
index a3c4e86..d67b9da 100644
--- a/lib/timevar.h
+++ b/lib/timevar.h
@@ -1,4 +1,4 @@
-/* Timing variables for measuring compiler performance.
+/* Timing variables for measuring application performance.
 
    Copyright (C) 2000, 2002, 2004, 2009-2015, 2018 Free Software
    Foundation, Inc.
@@ -22,7 +22,7 @@
 #define _TIMEVAR_H
 
 /* Timing variables are used to measure elapsed time in various
-   portions of the compiler.  Each measures elapsed user, system, and
+   portions of the application.  Each measures elapsed user, system, and
    wall-clock time, as appropriate to and supported by the host
    system.
 
@@ -86,7 +86,7 @@ void timevar_init (void);
 
    TIMEVAR cannot be running as a standalone timer.  */
 
-void timevar_push (timevar_id_t);
+void timevar_push (timevar_id_t /*timevar*/);
 
 /* Pop the topmost timing variable element off the timing stack.  The
    popped variable must be TIMEVAR.  Elapsed time since the that
@@ -94,28 +94,29 @@ void timevar_push (timevar_id_t);
    stack when the element above it was popped off, is credited to that
    timing variable.  */
 
-void timevar_pop (timevar_id_t);
+void timevar_pop (timevar_id_t /*timevar*/);
 
 /* Start timing TIMEVAR independently of the timing stack.  Elapsed
    time until timevar_stop is called for the same timing variable is
    attributed to TIMEVAR.  */
 
-void timevar_start (timevar_id_t);
+void timevar_start (timevar_id_t /*timevar*/);
 
 /* Stop timing TIMEVAR.  Time elapsed since timevar_start was called
    is attributed to it.  */
 
-void timevar_stop (timevar_id_t);
+void timevar_stop (timevar_id_t /*timevar*/);
+
 /* Fill the elapsed time for TIMEVAR into ELAPSED.  Returns
    update-to-date information even if TIMEVAR is currently running.  */
 
-void timevar_get (timevar_id_t, struct timevar_time_def *);
+void timevar_get (timevar_id_t /*timevar*/, struct timevar_time_def * /*elapsed*/);
 
 /* Summarize timing variables to FP.  The timing variable TV_TOTAL has
    a special meaning -- it's considered to be the total elapsed time,
    for normalizing the others, and is displayed last.  */
 
-void timevar_print (FILE *);
+void timevar_print (FILE * /*fp*/);
 
 /* Set to to nonzero to enable timing variables.  */
 extern int timevar_enabled;
diff --git a/modules/timevar b/modules/timevar
index 81a74b0..fdbbd6b 100644
--- a/modules/timevar
+++ b/modules/timevar
@@ -2,8 +2,8 @@ Description:
 A simple self-profiling module based on timers.
 
 Files:
-lib/timevar.c
 lib/timevar.h
+lib/timevar.c
 m4/timevar.m4
 
 Depends-on:
@@ -21,4 +21,4 @@ License:
 GPLv3+
 
 Maintainer:
-all
+Akim Demaille <akim.demaille@gmail.com>

Re: timevar: small tweaks

[Akim Demaille]

Hi Bruno,

> Le 30 sept. 2018 à 01:09, Bruno Haible <bruno@clisp.org> a écrit :
> 
> Hi Akim,
> 
> How about the following patches:
> 
> * In timevar.c: Include timevar.h first. This is a Gnulib best practice, which
>  has the benefit of verifying that the header file is self-contained.
> 
> * In timevar.def:
>  - The DEFTIMEVAR invocation must NOT be followed by a semicolon, otherwise
>    you'll get a syntax error in the enum definition.
>  - Enum values are not positive integers. The first enum value is 0.
>  - What is the difference between a "character string" and a "string"?
>  - The statement that the timing table is printed in the given order
>    contradicts the comment in timevar.h.
> 
> * In timevar.h:
>  - Better talk about the program or the application, not the « compiler".

All this is good, of course.

>  - It is pointless to omit the parameter names from the function declarations
>    if the comments talk about TIMEVAR, ELAPSED, FP, etc.

Here, I don’t understand why you commented the argument names.
It would be clearer with them uncommented.  I should have done
that when converting to ANSI C.

Also, maybe the extern C.  I see several modules have it.

> * In the module description:
>  Are you subscribed to bug-gnulib?

Nope.

> If not, you should be listed as the
>  maintainer of the modules, so that people CC you when they have questions
>  or patches about it.

Sure!

Re: timevar: small tweaks

[Bruno Haible]

Hi Akim,

Thanks for the review.

> >  - It is pointless to omit the parameter names from the function declarations
> >    if the comments talk about TIMEVAR, ELAPSED, FP, etc.
> 
> Here, I don’t understand why you commented the argument names.
> It would be clearer with them uncommented.

This is my preference as well. I thought the argument names had been removed
to avoid compilation errors when people define 'elapsed', 'fp', etc. as
C macros in their compilation units.

> Also, maybe the extern C.  I see several modules have it.

Good point, yes. In gnulib we add 'extern "C"' to header files upon request.
Feel free to add it.

> > If not, you should be listed as the
> >  maintainer of the modules, so that people CC you when they have questions
> >  or patches about it.
> 
> Sure!

OK, pushed as follows:


2018-09-30  Bruno Haible  <bruno@clisp.org>

	timevar: Small tweaks.
	* lib/timevar.h: Fix comments. Add parameter names to function
	declarations.
	* lib/timevar.c: Include timevar.h immediately after config.h.
	* lib/timevar.def: Fix comments.
	* modules/timevar (Maintainer): List Akim Demaille.

diff --git a/lib/timevar.c b/lib/timevar.c
index 0578816..a2d2433 100644
--- a/lib/timevar.c
+++ b/lib/timevar.c
@@ -20,6 +20,9 @@
 
 #include <config.h>
 
+/* Specification.  */
+#include "timevar.h"
+
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -101,8 +104,6 @@ static float clocks_to_msec;
 # define CLOCKS_TO_MSEC (1.0 / CLOCKS_PER_SEC)
 #endif
 
-#include "timevar.h"
-
 /* See timevar.h for an explanation of timing variables.  */
 
 int timevar_enabled = 0;
diff --git a/lib/timevar.def b/lib/timevar.def
index e5cc3ff..d640b30 100644
--- a/lib/timevar.def
+++ b/lib/timevar.def
@@ -24,21 +24,21 @@
 
    Syntax:
 
-     DEFTIMEVAR (Id, Name);
+     DEFTIMEVAR (Id, Name)
 
    where:
 
    - Id is the value used to identify the timing variable.
-     It's an enum value, i.e., behaves like a positive integer.
+     It's an enum value, i.e., behaves like a nonnegative integer.
      It is used only the manipulate the timer: the user of the
      program will never see this identifier.
 
-   - Name is a character string describing its purpose.  This string
+   - Name is a string describing its purpose.  This string
      will be presented to the user in the timing tables.  It does not
      need to be a literal, you may for instance use gettext.
 
    The order of this list matters: that is the order in which the
-   timing table is printed.
+   timing table is printed, except that 'tv_total' is printed last.
   */
 
 /* The total execution time.  Mandatory.  */
diff --git a/lib/timevar.h b/lib/timevar.h
index a3c4e86..88c4aa8 100644
--- a/lib/timevar.h
+++ b/lib/timevar.h
@@ -1,4 +1,4 @@
-/* Timing variables for measuring compiler performance.
+/* Timing variables for measuring application performance.
 
    Copyright (C) 2000, 2002, 2004, 2009-2015, 2018 Free Software
    Foundation, Inc.
@@ -22,7 +22,7 @@
 #define _TIMEVAR_H
 
 /* Timing variables are used to measure elapsed time in various
-   portions of the compiler.  Each measures elapsed user, system, and
+   portions of the application.  Each measures elapsed user, system, and
    wall-clock time, as appropriate to and supported by the host
    system.
 
@@ -86,7 +86,7 @@ void timevar_init (void);
 
    TIMEVAR cannot be running as a standalone timer.  */
 
-void timevar_push (timevar_id_t);
+void timevar_push (timevar_id_t timevar);
 
 /* Pop the topmost timing variable element off the timing stack.  The
    popped variable must be TIMEVAR.  Elapsed time since the that
@@ -94,28 +94,29 @@ void timevar_push (timevar_id_t);
    stack when the element above it was popped off, is credited to that
    timing variable.  */
 
-void timevar_pop (timevar_id_t);
+void timevar_pop (timevar_id_t timevar);
 
 /* Start timing TIMEVAR independently of the timing stack.  Elapsed
    time until timevar_stop is called for the same timing variable is
    attributed to TIMEVAR.  */
 
-void timevar_start (timevar_id_t);
+void timevar_start (timevar_id_t timevar);
 
 /* Stop timing TIMEVAR.  Time elapsed since timevar_start was called
    is attributed to it.  */
 
-void timevar_stop (timevar_id_t);
+void timevar_stop (timevar_id_t timevar);
+
 /* Fill the elapsed time for TIMEVAR into ELAPSED.  Returns
    update-to-date information even if TIMEVAR is currently running.  */
 
-void timevar_get (timevar_id_t, struct timevar_time_def *);
+void timevar_get (timevar_id_t timevar, struct timevar_time_def *elapsed);
 
 /* Summarize timing variables to FP.  The timing variable TV_TOTAL has
    a special meaning -- it's considered to be the total elapsed time,
    for normalizing the others, and is displayed last.  */
 
-void timevar_print (FILE *);
+void timevar_print (FILE *fp);
 
 /* Set to to nonzero to enable timing variables.  */
 extern int timevar_enabled;
diff --git a/modules/timevar b/modules/timevar
index 81a74b0..fdbbd6b 100644
--- a/modules/timevar
+++ b/modules/timevar
@@ -2,8 +2,8 @@ Description:
 A simple self-profiling module based on timers.
 
 Files:
-lib/timevar.c
 lib/timevar.h
+lib/timevar.c
 m4/timevar.m4
 
 Depends-on:
@@ -21,4 +21,4 @@ License:
 GPLv3+
 
 Maintainer:
-all
+Akim Demaille <akim.demaille@gmail.com>