On 11/25/14 7:56, Joseph Myers wrote:
> On Sun, 23 Nov 2014, Chen Gang wrote:
>
>> + gcc_assert (wi::fits_to_tree_p(value, integer_type_node));
>
> Watch formatting: space before '(' in the wi::fits_to_tree_p call.
> Applies elsewhere in this patch as well.
>
OK, thanks, I shall notice next.
> When making such an interface change, (a) you should update the comment on
> builtin_define_with_int_value to explain the new interface, and (b) you
> should check existing callers to make sure their values are indeed in
> range, and describe the check you did.
>
> In fact, -fabi-version=0 results in __GXX_ABI_VERSION being defined to
> 999999 using builtin_define_with_int_value. That's out of range of int on
> targets with 16-bit int. So that indicates against requiring the value to
> be within range of int. It might however be OK to require the value to be
> within range of target long.
>
For me, can let builtin_define_with_int_value() fit all kinds of integer
values, and the assert need be:
gcc_assert (wi::fits_to_tree_p (value, char_type_node)
|| wi::fits_to_tree_p (value, short_integer_type_node)
|| wi::fits_to_tree_p (value, integer_type_node)
|| wi::fits_to_tree_p (value, long_integer_type_node)
|| wi::fits_to_tree_p (value, long_long_integer_type_node));
If it really can fit all kinds of integer values, for me, the related
comments of builtin_define_with_int_value() need not be changed.
>> + if (value >= 0)
>> + {
>> + sprintf (buf, "%s="HOST_WIDE_INT_PRINT_DEC"%s",
>> + macro, value,
>> + value <= HOST_INT_MAX
>> + ? ""
>> + : value <= HOST_LONG_MAX
>> + ? "L" : "LL");
>
> Limits on the host's int and long are completely irrelevant here. The
> question is the target's int and long, not the host's - and consistency
> indicates checking with wi::fits_to_tree_p (value, integer_type_node) if
> the assertion checked with long_integer_type_node.
>
OK, thanks. And for me, the related sprintf() should be:
sprintf (buf, "%s=%s"HOST_WIDE_INT_PRINT_DEC"%s%s",
macro,
value < 0 ? "(" : "",
value,
wi::fits_to_tree_p (value, char_type_node)
|| wi::fits_to_tree_p (value, short_integer_type_node)
|| wi::fits_to_tree_p (value, integer_type_node)
? ""
: wi::fits_to_tree_p (value, long_integer_type_node)
? "L" : "LL",
value < 0 ? ")" : "");
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
