Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Joel Brobecker <brobecker@adacore.com>]

> How about the following cleanup:
> 
> Change producer_is_gcc function return type to bool.
> 
> gdb/ChangeLog:
> 
>         * utils.h (producer_is_gcc): Change return type to bool. Add major
>         argument.
>         * utils.c (producer_is_gcc): Likewise.
>         (producer_is_gcc_ge_4): Adjust producer_is_gcc call.
>         * dwarf2read.c (check_producer): Likewise.

It looks really great, thanks for doing that!

I have few very minor nits to report (see below), and also I'm wincing
a bit at the use of type bool. This is the first use in GDB, and
while I don't see that as a problem, and will pre-approve this patch,
let's have this patch sit for a week to give people the opportunity
to comment before we push it. Even if people don't comment before
we put it in, and give good reasons for using "int" instead, it is
easy to fix it afterwards, and I will take care of that.

We might have to adjust our gnulib import to include the "stdbool"
module, though...

Thank you,
-- 
Joel

> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 0d8026f..eb8541f 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -12293,7 +12293,7 @@ check_producer (struct dwarf2_cu *cu)
>  	 combination.  gcc-4.5.x -gdwarf-4 binaries have DW_AT_accessibility
>  	 interpreted incorrectly by GDB now - GCC PR debug/48229.  */
>      }
> -  else if ((major = producer_is_gcc (cu->producer, &minor)) > 0)
> +  else if (producer_is_gcc (cu->producer, &major, &minor))
>      {
>        cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
>        cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor < 3);
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 909476b..99b3874 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3259,7 +3259,8 @@ int
>  producer_is_gcc_ge_4 (const char *producer)
>  {
>    int major, minor;
> -  major = producer_is_gcc (producer, &minor);
> +  if (! producer_is_gcc (producer, &major, &minor))

Would you mind taking this opportunity for adding an empty line
after the declarations? It's a GDB Coding Style...

> +    return -1;
>    if (major < 4)
>      return -1;
>    if (major > 4)
> @@ -3267,17 +3268,22 @@ producer_is_gcc_ge_4 (const char *producer)
>    return minor;
>  }
>  
> -/* Returns the major version number if the given PRODUCER string is GCC and
> -   sets the MINOR version.  Returns -1 if the given PRODUCER is NULL or it
> -   isn't GCC.  */
> -int
> -producer_is_gcc (const char *producer, int *minor)
> +/* Returns true if the given PRODUCER string is GCC and sets the MAJOR
> +   and MINOR versions when not NULL.  Returns false if the given PRODUCER
> +   is NULL or it isn't GCC.  */
> +bool

Similar to above, we have a convention that function documentations
should be separated by an empty line from the implementation.

> +producer_is_gcc (const char *producer, int *major, int *minor)
>  {
>    const char *cs;
> -  int major;
>  
>    if (producer != NULL && strncmp (producer, "GNU ", strlen ("GNU ")) == 0)
>      {
> +      int maj, min;
> +      if (major == NULL)

Empty line after "int maj, min;"



> +	major = &maj;
> +      if (minor == NULL)
> +	minor = &min;
> +
>        /* Skip any identifier after "GNU " - such as "C11" "C++" or "Java".
>  	 A full producer string might look like:
>  	 "GNU C 4.7.2"
> @@ -3289,7 +3295,7 @@ producer_is_gcc (const char *producer, int *minor)
>          cs++;
>        if (*cs && isspace (*cs))
>          cs++;
> -      if (sscanf (cs, "%d.%d", &major, minor) == 2)
> +      if (sscanf (cs, "%d.%d", major, minor) == 2)
>  	return major;
>      }
>  
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 6724d7c..d8afa79 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -21,6 +21,8 @@
>  #ifndef UTILS_H
>  #define UTILS_H
>  
> +#include <stdbool.h>
> +
>  #include "exceptions.h"
>  
>  extern void initialize_utils (void);
> @@ -302,7 +304,7 @@ extern pid_t wait_to_die_with_timeout (pid_t pid, int *status, int timeout);
>  #endif
>  
>  extern int producer_is_gcc_ge_4 (const char *producer);
> -extern int producer_is_gcc (const char *producer, int *minor);
> +extern bool producer_is_gcc (const char *producer, int *major, int *minor);
>  
>  extern int myread (int, char *, int);
>  
> -- 
> 1.8.3.1
>