[PATCH] D19244: Extend checking of va_start builtin

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7417-7421
@@ -7416,4 +7416,7 @@
   InGroup<Varargs>;
-def warn_va_start_of_reference_type_is_undefined : Warning<
-  "'va_start' has undefined behavior with reference types">, InGroup<Varargs>;
+def warn_va_start_type_is_undefined : Warning<
+  "'va_start' has undefined behavior with "
+  "%select{types that undergo default argument promotion|reference types|"
+  "an object declared with the register storage class}0">,
+  InGroup<Varargs>;
 def err_first_argument_to_va_arg_not_of_type_va_list : Error<
----------------
aaron.ballman wrote:
> rsmith wrote:
> > I'm not sure that
> > 
> >     'va_start' has undefined behavior with an object declared with the register storage class
> > 
> > is clear enough about what object it's talking about. Can you explicitly state that the problem is with the type of the parameter name passed to `va_start` here, somehow?
> How about: "'va_start' has undefined behavior with a parameter declared with the 'register' keyword"?
> 
> Then again, we could reword the entire diagnostic and I wouldn't be sad. I notice that it's using the plural "types" already instead of the singular form, for instance. How about:
> 
> "Passing %select{an object of reference type|an object that undergoes default argument promotion|a parameter declared with the 'register' keyword}0 to 'va_start' is undefined behavior"
I think either of these is fine. (Though in the latter one, I'd say "has undefined behavior" or "results in undefined behavior" rather than "is undefined behavior".)

================
Comment at: lib/Sema/SemaChecking.cpp:2722
@@ -2720,2 +2721,3 @@
       ParamLoc = PV->getLocation();
+      IsRegister = PV->getStorageClass() == SC_Register;
     }
----------------
aaron.ballman wrote:
> rsmith wrote:
> > Where is this restriction specified? Does this only apply to C?
> 7.16.1.4p3:
> 
> If the parameter parmN is declared with the register storage class ...  the behavior is undefined.
> 
> I think this may only apply in C, actually, because [support.runtime]p3 does not mention it in its description (even as far back as C++03). Whether that's an explicit omission (to make va_start() more powerful than in C) or an oversight is kind of moot since register was removed in C++17. Thoughts?
I think it's an intentional difference with C. The intent of disallowing `register` is to allow `va_start` to be implemented as a macro that takes the address of the parameter. This would be ill-formed in C for a `register` parameter, whereas in C++ it is (was) valid to take the address of a variable declared with the `register` specifier.


http://reviews.llvm.org/D19244