This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Ping - old patch from April - mingw support for I32/I64 MS printf formatters to c-format.c
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Kai Tietz <Kai dot Tietz at onevision dot com>
- Cc: Danny Smith <dansmister at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, NightStrike <nightstrike at gmail dot com>
- Date: Wed, 13 Feb 2008 18:17:01 +0000 (UTC)
- Subject: Re: Ping - old patch from April - mingw support for I32/I64 MS printf formatters to c-format.c
- References: <OFB48CCF55.D0869FC4-ONC12573E5.004CDDD5-C12573E5.004EE0DB@onevision.de>
On Mon, 4 Feb 2008, Kai Tietz wrote:
> Thank you for your advice. May just by consistancy we should'nt declare
> C99 functions for c90 in format.h. But the %Y is not present for mingw at
> all. Therefore I prefer to disable them for these targets completely.
In that case you'll need to disable c99-printf-3.c for those targets as
well; this patch version still has the bogus changes there.
"formatter" is nowhere used in the existing code to refer to kinds of
formats; I suggest just using "format" throughout the patch.
> * testsuite/gcc.dg/format/sys_formatter.c: New.
Does not belong in gcc/; you appear to have a properly located ChangeLog
entry for this file below.
> @@ -1776,7 +1782,22 @@ check_format_info_main (format_check_res
> if (fli)
> {
> while (fli->name != 0 && fli->name[0] != *format_chars)
> - fli++;
> + {
> + if (fli->name[0] == '\0')
[...]
If this patch chunk is changing the meaning of some structure in
c-format.h, the comments on that structure need updating to describe the
new semantics implemented by this new code.
> +/* Description of gnu specific format attributes reflected to
> + system format attribute types, as printf, scanf, strftime, and
> + strfmon. */
Not clear English, especially "reflected". I think something like
/* Attributes such as "printf" are equivalent to those such as
"gnu_printf" unless this is overridden by a target. */
would be a clearer description of the purpose of this array.
> +const target_ovr_attr gnu_target_overrides_format_attributes[] =
I think this can be static.
> +/* Translate to unified attribute name. This is used in decode_format_type and
> + decode_format_attr. In attr_name the user specified argument is passed. It
> + returns the unified formatter name from TARGET_OVERRIDES_FORMAT_ATTRIBUTES
> + or the attr_name passed to this function, if there is no matching entry. */
> +static const char *
> +replace_formatter_name_to_system_name (const char *attr_name)
"replace ... to" isn't idiomatic English; "convert" might be better than
"replace" here.
> +/* A helper function to compare the target override format attribute tattr_name
> + and the user format attribute attr_name. The underscore variant of attr_name
> + is ignore for compare. I returns for equal attribute the value one, otherwise
> + zero. */
/* Return true if TATTR_NAME and ATTR_NAME are the same format attribute,
counting "name" and "__name__" as the same, false otherwise. */
> +static int
Should return bool rather than int.
> +/* For none gnu style formatter types we need to compare the none gnu formatter
> + and a gnu style formatter style to compare in kind. This is done via the
> + array TARGET_OVERRIDES_FORMAT_ATTRIBUTES. The argument custom_type specifiers
> + the index of the formatter. The argument def_type specifies the gnu style
> + formatter index to be compare with. If the kind is equal it returns true,
> + otherwise false. */
> +static int
What should this return for comparing each pair of strftime, gnu_strftime,
ms_strftime? I can't tell from the comment.
Actually, I don't think we need this function. The comparisons against
specific types that you change to call this function are used for two
things: processing GCC-internal formats, and giving the error "strftime
formats cannot format arguments". For the former, there will be no
target-specific variants; these formats are the same everywhere. For the
latter, it would be better to replace the hardcoded check for
strftime_format_type with a check for the format not setting
FMT_FLAG_ARG_CONVERT in its flags; that's the right condition to check.
The diagnostic should name the particular attribute in use, whether
"strftime", "gnu_strftime" or "ms_strftime" (that is, the attribute in the
user's source code, not any translated version of it).
> +/* Structure describing the target specific to be override formatter
> + attributes, e.g. printf, scanf, etc. This allows to support different
> + runtime-library specific formatter attributes to co-exist and defining
> + a default system version.
> + This type is used for the pointer variable TARGET_OVERRIDES_FORMAT_ATTRIBUTES
> + refers to. */
/* Structure describing how format attributes such as "printf" are
interpreted as "gnu_printf" or "ms_printf" on a particular system.
TARGET_OVERRIDES_FORMAT_ATTRIBUTES is used to specify target-specific
defaults. */
> +/* Be aware to keep these values in synch with enum format_type from c-format.c. */
Perhaps that enum should simply move to this header and avoid the separate
list?
--
Joseph S. Myers
joseph@codesourcery.com