Re: [PATCH] integer overflow checking builtins in constant expressions

[Martin Sebor <msebor at gmail dot com>]

> > > 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 see.  You meant that only the clang compatibility built-ins
(i.e. the typed ones like  __builtin_uadd_overflow) shouldn't
be allowed to take null pointer as the last argument, but the
type-generic ones should.  That would work for C, though it
won't satisfy the feature request in c/68120 which asks for
the last argument to be optional (it can't be there since it
determines the type of the operation).  It will not work for
C++ 98.  But those might be acceptable limitations (at least
in C there's a workaround).

> ?
> I don't get any warning/error on
> #include <stddef.h>
> int *const p = (int *) NULL;
> in C89 nor C++98 with -pedantic-errors,

That's because the above is not a constant expression.  If such
a cast were to be used in one like the enum below, GCC in C++ 98
mode would give:

  error: a cast to a type other than an integral or enumeration type 
cannot appear in a constant-expression
   enum { e = (long)(int*)0 };
                          ^

(Curiously, this is accepted with a pedantic warning in C mode.)

> 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.

Since GCC accepts the above as an extension it was tempting
to change G++ to accept it as well.  But such a change seemed
beyond the scope of this patch.  Applying different type system
rules in a set of built-ins than in the rest of the language
wouldn't seem appropriate to me.  I would rather accept the
limitation that the typed built-ins won't be usable in C++ 98
unless Jason recommends to go down that route.

Unless I hear otherwise, I'll go ahead and change the patch
to have only the type-generic built-ins accept a null pointer
as an argument.

> > 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.

I agree that independent substantive changes are best made
separately.

I made this one part of the patch because it affects the tests
that go with it (that's how I found the inconsistency, by my
tests failing), and because changing the spelling of a warning
seemed too trivial of a change to justify a patch of its own
and taking up all of your time to review and approve it.  But
since you insist I will submit it separately ahead of this
updated patch.

Martin