Re: [PATCH] gdb: Use C++11 std::chrono

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2016-11-17 12:15, Pedro Alves wrote:
> This patch fixes a few problems with GDB's time handling.
> [...]

Indeed, it makes the code very nice and readable.  My only opinion about 
std::chrono so far was that it was so much more painful to do 
std::this_thread::sleep_for(std::chrono::seconds(2)) than sleep(2). :)

> +/* Count the total amount of time spent executing in user mode.  */
>
> +user_cpu_time_clock::time_point
> +user_cpu_time_clock::now () noexcept
> +{
> +  return time_point (microseconds (get_run_time ()));

Looking at get_run_time() in libiberty, it seems like it returns user 
time + system time.  Doesn't it contradict the naming of this clock and 
the comment of this method?

> -      /* If gettimeofday doesn't exist, and as a portability solution 
> it has
> -	 been replaced with, e.g., time, then it doesn't make sense to print
> -	 the microseconds field.  Is there a way to check for that?  */
> -      fprintf (stderr, "%ld:%06ld ", (long) tm.tv_sec, (long) 
> tm.tv_usec);
> +      fprintf (stderr, "%ld:%06ld ", s.count (), us.count ());

Unrelated to your change, but I find it weird to format the time as 
"seconds:useconds".  I wouldn't object if you sneakily changed it to 
"seconds.useconds". :)

> @@ -2390,8 +2383,8 @@ mi_load_progress (const char *section_name,
>  		  unsigned long total_sent,
>  		  unsigned long grand_total)
>  {
> -  struct timeval time_now, delta, update_threshold;
> -  static struct timeval last_update;
> +  using namespace std::chrono;
> +  static steady_clock::time_point last_update;
>    static char *previous_sect_name = NULL;
>    int new_section;
>    struct ui_out *saved_uiout;
> @@ -2416,18 +2409,9 @@ mi_load_progress (const char *section_name,
>
>    uiout = current_uiout;
>
> -  update_threshold.tv_sec = 0;
> -  update_threshold.tv_usec = 500000;
> -  gettimeofday (&time_now, NULL);
> -
> -  delta.tv_usec = time_now.tv_usec - last_update.tv_usec;
> -  delta.tv_sec = time_now.tv_sec - last_update.tv_sec;
> -
> -  if (delta.tv_usec < 0)
> -    {
> -      delta.tv_sec -= 1;
> -      delta.tv_usec += 1000000L;
> -    }
> +  microseconds update_threshold (500000);
> +  steady_clock::time_point time_now = steady_clock::now ();
> +  steady_clock::duration delta = time_now - last_update;

Can this be simplified to avoid having the delta variable?  Since we can 
easily compare two time_points, we can probably make it look like:

  if (steady_clock::now () >= next_update)
    {
      next_update = steady_clock::now() + update_threshold;
      ...
    }

or

  if (steady_clock::now () >= (last_update + update_threshold))
    {
      last_update = steady_clock::now ();
      ...
    }

Also, wouldn't "update_period" or "update_rate" be more precise than 
"update_threshold"?

>  static void
>  timestamp (struct mi_timestamp *tv)
>  {
> -  gettimeofday (&tv->wallclock, NULL);
> -#ifdef HAVE_GETRUSAGE
> -  getrusage (RUSAGE_SELF, &rusage);
> -  tv->utime.tv_sec = rusage.ru_utime.tv_sec;
> -  tv->utime.tv_usec = rusage.ru_utime.tv_usec;
> -  tv->stime.tv_sec = rusage.ru_stime.tv_sec;
> -  tv->stime.tv_usec = rusage.ru_stime.tv_usec;
> -#else
> -  {
> -    long usec = get_run_time ();
> -
> -    tv->utime.tv_sec = usec/1000000L;
> -    tv->utime.tv_usec = usec - 1000000L*tv->utime.tv_sec;
> -    tv->stime.tv_sec = 0;
> -    tv->stime.tv_usec = 0;
> -  }
> -#endif
> +  using namespace std::chrono;

It's not written in the coding style guide, but I think it would be nice 
to have a newline between the "using" declaration and the following 
lines, like we do with variable declarations.

> -/* Report how fast the transfer went.  */
> +/* Report on STREAM the performance of a memory transfer operation,
> +   such as 'load'.  DATA_COUNT is the number of bytes transferred.
> +   WRITE_COUNT is the number of separate write operations, or 0, if
> +   that information is not available.  TIME is the range of time in
> +   which the operation lasted.  */
>
> -void
> +static void
>  print_transfer_performance (struct ui_file *stream,
>  			    unsigned long data_count,
>  			    unsigned long write_count,
> -			    const struct timeval *start_time,
> -			    const struct timeval *end_time)
> +			    const time_range &time)

Is there a reason you couldn't use a std::chrono::duration instead of a 
time_range here?  The function doesn't seem to care about the particular 
time_points, only their difference.

> -      timestamp = xstrprintf ("%ld:%ld %s%s",
> -			      (long) tm.tv_sec, (long) tm.tv_usec,
> -			      linebuffer,
> -			      need_nl ? "\n": "");
> -      make_cleanup (xfree, timestamp);
> -      fputs_unfiltered (timestamp, stream);
> +      std::string timestamp = string_printf ("%ld:%ld %s%s",

Same comment about secs:usecs vs secs.usecs.