On Tue, May 31, 2016 at 04:44:45PM -0600, Martin Sebor wrote:
> >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.
Ugh, I've missed missing break; Anyway, IMHO we shouldn't change
the old style builtins and therefore you really don't need this.
> >>+ /* 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).
Sorry, I don't understand this argument. What is wrong on using
const int x = __builtin_add_overflow (1234567, 123456, (int *) NULL);
?
I don't get any warning/error on
#include <stddef.h>
int *const p = (int *) NULL;
in C89 nor C++98 with -pedantic-errors, not to mention that the
builtins are extensions anyway, so what do you accept/reject in its
arguments doesn't need to honor any specific rules. Even if (int *) NULL
is not allowed for whatever reason, is (int *) 0 disallowed too? It is
just a matter of what we document.
The clang compatibility builtins are severely limited and too ugly to live
with, they don't allow different types of the arguments, have just a very
limited set of accepted types, don't allow different result
type, how do you even handle a type for which you don't know if it is
unsigned, unsigned long or unsigned long long?
size_t a, b, c, d;
...
if (sizeof (size_t) == sizeof (unsigned long long))
a = __builtin_uaddll_overflow (b, c, &d);
else if (sizeof (size_t) == sizeof (unsigned long))
a = __builtin_uaddl_overflow (b, c, &d);
else if (sizeof (size_t) == sizeof (unsigned int))
a = __builtin_uadd_overflow (b, c, &d);
else
abort ();
? We really shouldn't encourage these at all.
> 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.
Pointers are treated the same as integers, they are represented as
INTEGER_CSTs too.
>
> >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);
Yeah. IMO that is an independent change, you can/should add a testcase for
it, and it can go in before or after the patch.
Jakub
