Re: [PATCH] dwarf2read: move producers to producers.c/h file

[Pedro Alves <palves at redhat dot com>]

On 09/21/2017 05:19 PM, Walfred Tedeschi wrote:
> This patch add new files to add dwarf reader utilities into it.

This sentence is stale.  It adds new files for the producer checks,
not for DWARF reader utilities.

> It is also a preparation path to add functions to detect the icc
> version that produced a given compilation unit.
> 
> 2017-09-18  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* Makefile.in (SFILES): Add dwarf2utils.c.
> 	(COMMON_OBS): Add dwarf2utils.o

Also stale.

> 	* amd64-tdep.c (producer.h): Add new include.
> 	* dwarf2read.c (producer.h): Add new include.
> 	* producer.c: New file.
> 	* producer.h: New file.
> 	* utils.c (producer_is_gcc, producer_is_gcc_ge_4): Move to
> 	producer.c.
> 	* utils.h (producer_is_gcc, producer_is_gcc_ge_4): Move to
> 	producer.h.
> 

> +++ b/gdb/producer.c
> @@ -0,0 +1,126 @@
> +/* Producer for GDB.

This may be a bit too mysterious for readers.

How about:

Producer version strings parsing, for GDB.

> +
> +   This file is part of GDB.
> +

Missing the copyright date line.

> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "config.h"
> +#include "producer.h"
> +#include "stdio.h"
> +#include "string.h"
> +#include "defs.h"
> +#include "dyn-string.h"
> +#include <ctype.h>
> +#include "common/common-types.h"
> +#include "common/common-exceptions.h"
> +#include "common/common-utils.h"
> +#include "utils.h"

This includes list needs to be cleaned up substantially.

defs.h goes first, then producer.h, then the rest.  You
shouldn't include config.h directly.  "stdio.h" and "string.h"
using "" instead of <> look odd.  I expect that many of those
you don't actually need.

Please take a look here:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Include_Files

> +/* See documentation in the producer.h file.  */
> +
> +bool
> +producer_is_icc (const char *producer, int *major, int *minor)
> +{

This is new code that should be in the follow up patch ...

> +++ b/gdb/producer.h
> @@ -0,0 +1,36 @@
> +/* Producer for GDB.

See above.

> +   Copyright (C) 2017 Free Software Foundation, Inc.

This is not new code, so we should preserve the copyright
years.  A git blame on dwarf2read.c seems to indicate these
functions were added in 2012, so write 2012-2017.  Likewise
in the header.

> +#ifndef PRODUCER_H
> +#define PRODUCER_H
> +
> +/* Check for GCC >= 4.x according to the symtab->producer string.  Return minor
> +-  version (x) of 4.x in such case.  If it is not GCC or it is GCC older than
> +-  4.x return -1.  If it is GCC 5.x or higher return INT_MAX.  */

Please remove the strange leading '-'s that you're introducing.

> +
> +extern int producer_is_gcc_ge_4 (const char *producer);

No space between comment and declaration in header declarations.
This is described in the same url I pointed at above.

> +
> +
> +/* Returns nonzero if the given PRODUCER string is GCC and sets the MAJOR
> +   and MINOR versions when not NULL.  Returns zero if the given PRODUCER
> +   is NULL or it isn't GCC.  */
> +
> +extern int producer_is_gcc (const char *producer, int *major, int *minor);

Ditto.

Thanks,
Pedro Alves