Re: [PATCH] icc: allow code path for newer versions of icc.

[Pedro Alves <palves at redhat dot com>]

Hi,

I read the patch and I found a few nits to pick.
See below.

On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 6678b33..4aa3b3e 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -604,7 +604,7 @@ struct dwarf2_cu
>    unsigned int checked_producer : 1;

...

>  
> +#if defined GDB_SELF_TEST
> +#include "selftest.h"
> +
> +namespace selftests {
> +namespace gdbserver {
> +static void
> +dwarf_producer_test ()

Simon already pointed at the gdbserver namespace.  I'd like
to add that it looks odd to me to define the actual
functionality in utils.c and define the corresponding self
tests in dwarf2read.c.  Why not put the tests in utils.c as well?

BTW, I'd support moving all these producer checks to
a separate file (maybe producer.c), instead of putting
it all in the utils.c kitchen sync, though that's a
preexisting issue.

> +  if (producer == NULL || !startswith (producer, "Intel(R)"))
> +    return 0;
> +
> +/* Preparing the used fields.  */
> +  int maj, min;

Missing leading double-space in comment line.

> +  if (major == NULL)
> +    major = &maj;
> +  if (minor == NULL)
> +    minor = &min;

> +  /* Not recognized as Intel, let user know.  */
> +  warning ("Intel producer: version not recognized.");
> +  return 0;
> +}

If this ever triggers, won't the user get an annoying
flood of such warnings?  If you hack your local copy to reach
the warning, what do you see?

I also wonder whether we should print a more-friend clearer
message, "intel producer" may mean nothing to a user, and I'm
not sure what they could do with that.  Maybe at least print
the actual version string?

Thanks,
Pedro Alves