[PATCH] D13263: Addition of __attribute__((pass_object_size)) to Clang

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2068
@@ -2064,1 +2067,3 @@
 def err_attribute_pointers_only : Error<warn_attribute_pointers_only.Text>;
+def err_attribute_constant_pointers_only : Error<
+  "%0 attribute only applies to constant pointer arguments">;
----------------
Instead of making a new diagnostic, I think it better to modify warn_attribute_pointers_only to use a %select statement (then err_attribute_pointers_only also gets updated). I would separate that into a prequel patch since it's likely to introduce a fair amount of unrelated changes to this patch.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:820
@@ +819,3 @@
+  Expr *E = Attr.getArgAsExpr(0);
+  IntegerLiteral *Int = dyn_cast<IntegerLiteral>(E->IgnoreParenCasts());
+  if (Int == nullptr) {
----------------
We don't usually ignore parens and casts for attributes like this for other attributes. Is that important for you uses for some reason?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:828
@@ +827,3 @@
+  auto APType = Int->getValue();
+  if (APType.ugt(3) || APType.slt(0)) {
+    S.Diag(E->getLocStart(), diag::err_attribute_argument_out_of_bounds)
----------------
rsmith wrote:
> The second check here is redundant if `APType` is more than 2 bits wide (and looks wrong when `Int` is of type `bool`).
I'm wondering why the magic value 3 at all? It seems like this (and the above code) should be replaced by a call to checkFunctionOrMethodParameterIndex().

================
Comment at: lib/Sema/SemaDeclAttr.cpp:828
@@ +827,3 @@
+  auto APType = Int->getValue();
+  if (APType.ugt(3) || APType.slt(0)) {
+    S.Diag(E->getLocStart(), diag::err_attribute_argument_out_of_bounds)
----------------
aaron.ballman wrote:
> rsmith wrote:
> > The second check here is redundant if `APType` is more than 2 bits wide (and looks wrong when `Int` is of type `bool`).
> I'm wondering why the magic value 3 at all? It seems like this (and the above code) should be replaced by a call to checkFunctionOrMethodParameterIndex().
There should be comments explaining the magic number 3.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:829
@@ +828,3 @@
+  if (APType.ugt(3) || APType.slt(0)) {
+    S.Diag(E->getLocStart(), diag::err_attribute_argument_out_of_bounds)
+      << Attr.getName() << 1 << E->getSourceRange();
----------------
That isn't the correct diagnostic to emit. That one is used when a function attribute that refers to a parameter (like format_arg) uses an index that does not refer to a parameter. For this one, I would probably modify err_attribute_argument_outof_range to not be specific to 'init_priority'.


http://reviews.llvm.org/D13263