Re: [PATCH 01/15 v2] Introduce common/errors.h

[Doug Evans <dje at google dot com>]

On Wed, Jul 16, 2014 at 7:17 AM, Gary Benson <gbenson@redhat.com> wrote:
> This introduces common/errors.h.  This holds some error- and
> warning-related declarations that can be used by the code in common.
> Clients of the "common" code must provide definitions for these
> functions.
>
> gdb/
> 2014-07-16  Tom Tromey  <tromey@redhat.com>
>             Gary Benson  <gbenson@redhat.com>
>
>         * common/errors.h: New file.
>         * utils.h: Include errors.h.
>         (perror_with_name, error, fatal, warning): Don't declare.
>         * common/common-utils.h: Include errors.h.
>         (malloc_failure, internal_error): Don't declare.
>
> gdb/gdbserver/
> 2014-07-16  Tom Tromey  <tromey@redhat.com>
>
>         * utils.h: Include errors.h.
>         (perror_with_name, error, fatal, warning): Don't declare.

Hi.  Comments inline.

> ---
>  gdb/ChangeLog             |    9 +++++++
>  gdb/common/common-utils.h |    5 +---
>  gdb/common/errors.h       |   55 +++++++++++++++++++++++++++++++++++++++++++++
>  gdb/gdbserver/ChangeLog   |    5 ++++
>  gdb/gdbserver/utils.h     |    5 +---
>  gdb/utils.h               |   10 +-------
>  6 files changed, 72 insertions(+), 17 deletions(-)
>  create mode 100644 gdb/common/errors.h
>
> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
> index 063698d..53be2f8 100644
> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -24,6 +24,7 @@
>  #include "ansidecl.h"
>  #include <stddef.h>
>  #include <stdarg.h>
> +#include "errors.h"
>
>  /* If possible, define FUNCTION_NAME, a macro containing the name of
>     the function being defined.  Since this macro may not always be
> @@ -43,10 +44,6 @@
>  #endif
>  #endif
>
> -extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
> -extern void internal_error (const char *file, int line, const char *, ...)
> -     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
> -
>  /* xmalloc(), xrealloc() and xcalloc() have already been declared in
>     "libiberty.h". */
>
> diff --git a/gdb/common/errors.h b/gdb/common/errors.h
> new file mode 100644
> index 0000000..5f998e3
> --- /dev/null
> +++ b/gdb/common/errors.h
> @@ -0,0 +1,55 @@
> +/* Declarations for error-reporting facilities.
> +
> +   Copyright (C) 1986-2014 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#ifndef COMMON_ERRORS_H
> +#define COMMON_ERRORS_H
> +
> +/* The declarations in this file are, for the time being, separately
> +   implemented by gdb and gdbserver.  However they share a common
> +   definition so that they can be used by code in common/.  */
> +
> +/* Like "perror" but throws an exception with the appropriate
> +   text.  */
> +
> +extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
> +
> +/* Throw an exception.  */
> +
> +extern void error (const char *fmt, ...)
> +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> +
> +/* Cause a fatal error.  */
> +
> +extern void fatal (const char *fmt, ...)
> +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);

fatal is a confusing name IMO.  In gdb it just throws RETURN_QUIT.
That's not your fault of course, but I wonder if I can trouble you to
expand on the function comment a bit.  E.g., something like the
following?

/* Cause a fatal error.
   What this does in gdb is throw a RETURN_QUIT exception.  */

One could expand on the comment, but I didn't want to turn it into a FIXME.
[which is really what it should be, but that's a topic for another patch :-)]

> +
> +/* Issue a warning.  */
> +
> +extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
> +
> +/* Internal error.  */
> +
> +extern void internal_error (const char *file, int line, const char *, ...)
> +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
> +
> +/* Memory allocation failure.  */
> +
> +extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
> +
> +#endif /* COMMON_ERRORS_H */
> diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
> index 906924b..819fa35 100644
> --- a/gdb/gdbserver/utils.h
> +++ b/gdb/gdbserver/utils.h
> @@ -20,11 +20,8 @@
>  #define UTILS_H
>
>  #include "print-utils.h"
> +#include "errors.h"
>
> -void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
> -void error (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> -void fatal (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> -void warning (const char *string,...) ATTRIBUTE_PRINTF (1, 2);
>  char *paddress (CORE_ADDR addr);
>  char *pfildes (gdb_fildes_t fd);
>
> diff --git a/gdb/utils.h b/gdb/utils.h
> index a91f551..d9fe7e3 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -24,6 +24,7 @@
>  #include "cleanups.h"
>  #include "exceptions.h"
>  #include "print-utils.h"
> +#include "errors.h"
>
>  extern void initialize_utils (void);
>
> @@ -269,7 +270,6 @@ extern void fprintf_symbol_filtered (struct ui_file *, const char *,
>
>  extern void throw_perror_with_name (enum errors errcode, const char *string)
>    ATTRIBUTE_NORETURN;
> -extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
>
>  extern void perror_warning_with_name (const char *string);
>
> @@ -286,17 +286,11 @@ extern char *warning_pre_print;
>  extern void verror (const char *fmt, va_list ap)
>       ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
>
> -extern void error (const char *fmt, ...)
> -     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> -
>  extern void error_stream (struct ui_file *) ATTRIBUTE_NORETURN;
>
>  extern void vfatal (const char *fmt, va_list ap)
>       ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
>
> -extern void fatal (const char *fmt, ...)
> -     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> -

It's odd to separate out fatal and not vfatal.
[ditto for the others]
Can you add vfatal, et.al. to this patch?

>  extern void internal_verror (const char *file, int line, const char *,
>                              va_list ap)
>       ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
> @@ -308,8 +302,6 @@ extern void internal_vwarning (const char *file, int line,
>  extern void internal_warning (const char *file, int line,
>                               const char *, ...) ATTRIBUTE_PRINTF (3, 4);
>
> -extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
> -
>  extern void vwarning (const char *, va_list args) ATTRIBUTE_PRINTF (1, 0);
>
>  extern void demangler_vwarning (const char *file, int line,
> --
> 1.7.1
>