On 05/31/2016 03:50 PM, Jakub Jelinek wrote:
On Sun, May 01, 2016 at 10:39:48AM -0600, Martin Sebor wrote:
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -7878,50 +7878,101 @@ fold_builtin_unordered_cmp (location_t loc, tree
fndecl, tree arg0, tree arg1,
static tree
fold_builtin_arith_overflow (location_t loc, enum built_in_function fcode,
- tree arg0, tree arg1, tree arg2)
+ tree arg0, tree arg1, tree arg2 = NULL_TREE)
{
enum internal_fn ifn = IFN_LAST;
- tree type = TREE_TYPE (TREE_TYPE (arg2));
- tree mem_arg2 = build_fold_indirect_ref_loc (loc, arg2);
+ enum tree_code opcode;
+
+ /* For the "generic" overloads, the first two arguments can have different
+ types and the last argument determines the target type to use to check
+ for overflow. The names of each of the other overloads determines
+ the target type and so the last argument can be omitted. */
+ tree type = NULL_TREE;
+
switch (fcode)
{
case BUILT_IN_ADD_OVERFLOW:
+ type = type ? type : TREE_TYPE (TREE_TYPE (arg2));
case BUILT_IN_SADD_OVERFLOW:
+ type = type ? type : integer_type_node;
case BUILT_IN_SADDL_OVERFLOW:
+ type = type ? type : long_integer_type_node;
case BUILT_IN_SADDLL_OVERFLOW:
+ type = type ? type : long_long_integer_type_node;
case BUILT_IN_UADD_OVERFLOW:
+ type = type ? type : unsigned_type_node;
case BUILT_IN_UADDL_OVERFLOW:
+ type = type ? type : long_unsigned_type_node;
case BUILT_IN_UADDLL_OVERFLOW:
+ type = type ? type : long_long_unsigned_type_node;
ifn = IFN_ADD_OVERFLOW;
+ opcode = PLUS_EXPR;
...
I fail to understand this. You set type to NULL_TREE, and then (not in any
kind of loop, nor any label you could goto to) do:
type = type ? type : xxx;
That is necessarily equal to type = xxx;, isn't it?
Each label falls through to the next, so the test prevents
the subsequent labels from overwriting the value assigned
to type by the label that matched the switch expression.
I'm not exactly enamored with these repetitive tests but
the only alternative that occurred to me was to duplicate
the whole switch statement, and this seemed like the lesser
of the two evils. If you have a suggestion for something
better I'd be happy to change it.
+ /* When the last argument is a NULL pointer and the first two arguments
+ are constant, attempt to fold the built-in call into a constant
+ expression indicating whether or not it detected an overflow but
+ without storing the result. */
+ if ((!arg2 || zerop (arg2))
As I said in another mail, IMNSHO you don't want to change the clang
compatibility builtins, therefore arg2 should never be NULL.
Allowing the last argument to be null is the subject of
the enhancement request in c/68120 (one of the two PRs
driving this enhancement). The argument can only be null
for these overloads and not the type-generic ones because
the latter use the type to which the pointer points to
determine the type in which to do the arithmetic. Without
this change, C or C++ 98 users wouldn't be able to use the
built-ins in constant expressions (C++ 98 doesn't allow
casting a null pointer in those contexts).
As it is a pointer, shouldn't the above be integer_zerop instead?
I suspect I didn't use integer_zerop because the argument
is a pointer and not integer, but I'll change it before
I commit the patch.
Regarding the varargs vs. named args only different diagnostics, if you
think it should change, then IMHO you should submit as independent patch
a change to the diagnostics wording, so that the wording is the same, rather
than changing functions from fixed arguments to varargs (/ typegeneric).
I'm not sure I know what part of the patch you're referring
to. Are you suggesting to submit a separate patch for the
change from "not enough arguments" to "too few arguments"
below?
- "not enough arguments to function %qE", fndecl);
+ "too few arguments to function %qE", fndecl);
Martin
