[PATCH] D19244: Extend checking of va_start builtin

[Aaron Ballman via cfe-commits]

aaron.ballman 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<
----------------
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"

================
Comment at: lib/Sema/SemaChecking.cpp:2722
@@ -2720,2 +2721,3 @@
       ParamLoc = PV->getLocation();
+      IsRegister = PV->getStorageClass() == SC_Register;
     }
----------------
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?


http://reviews.llvm.org/D19244