[PATCH] D14274: Add alloc_size attribute to clang

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/Basic/Attr.td:714
@@ +713,3 @@
+  let Spellings = [GCC<"alloc_size">];
+  let Subjects = SubjectList<[Function]>;
+  let Args = [IntArgument<"ElemSizeParam">, IntArgument<"NumElemsParam", 1>];
----------------
Should this be using HasFunctionProto instead of Function?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2144
@@ -2143,1 +2143,3 @@
 def err_attribute_pointers_only : Error<warn_attribute_pointers_only.Text>;
+def warn_attribute_integers_only : Warning<
+  "%0 attribute only applies to integer arguments">,
----------------
This warning diagnostic is not used.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2145
@@ +2144,3 @@
+def warn_attribute_integers_only : Warning<
+  "%0 attribute only applies to integer arguments">,
+  InGroup<IgnoredAttributes>;
----------------
This text is a bit misleading based on the diagnostic. The attribute doesn't apply to the integer argument. The attribute argument should refer to an integer parameter of the function declaration. I think the diagnostic should reflect that. Perhaps:

%0 attribute argument may only refer to a function parameter of integer type

and the diagnostic loc can point to the attribute argument in question. (If that's not possible, it would be good to add a %1 to identify which of the two possible attribute arguments is at fault.)

================
Comment at: lib/AST/ExprConstant.cpp:117
@@ +116,3 @@
+  }
+  /// Attempts to unwrap a CallExpr (with an alloc_size attribute) from an Expr.
+  /// This will look through a single cast.
----------------
Add a newline to separate the functions (here and elsewhere).

================
Comment at: lib/AST/ExprConstant.cpp:128
@@ +127,3 @@
+    // probably be a cast of some kind. Ignore it.
+    if (auto *Cast = dyn_cast<CastExpr>(E))
+      E = Cast->getSubExpr()->IgnoreParens();
----------------
const auto *, here and elsewhere. Whenever you have auto, we want all the cv-qualifiers as well as */& information along with the auto.

================
Comment at: lib/AST/ExprConstant.cpp:144
@@ +143,3 @@
+  /// array in its designator.
+  static bool baseRequiresUnsizedArrayInDesignator(APValue::LValueBase Base) {
+    return isBaseAnAllocSizeCall(Base);
----------------
Is this function going to expand to be more than just a forward to another? If not, we should remove it.

================
Comment at: lib/AST/ExprConstant.cpp:156
@@ -119,1 +155,3 @@
+    // arrays that lack size info.
+    assert(!baseRequiresUnsizedArrayInDesignator(Base));
     unsigned MostDerivedLength = 0;
----------------
Would be good to have '&& "explanation of assert"' as well (here and elsewhere).

================
Comment at: lib/AST/ExprConstant.cpp:264
@@ +263,3 @@
+    /// Determined whether the most derived subobject is an array without a
+    /// known bound
+    bool isMostDerivedAnUnsizedArray() const {
----------------
Missing a period.

================
Comment at: lib/AST/ExprConstant.cpp:269
@@ +268,3 @@
+
+    uint64_t getMostDerivedArraySize() const {
+      assert(!isMostDerivedAnUnsizedArray());
----------------
Missing function documentation.

================
Comment at: lib/AST/ExprConstant.cpp:357
@@ +356,3 @@
+        // Can't verify -- trust that the user is doing the right thing (or if
+        // not, trust that the caller will catch the bad behavior)
+        Entries.back().ArrayIndex += N;
----------------
Missing period.

================
Comment at: lib/AST/ExprConstant.cpp:1109
@@ -1022,1 +1108,3 @@
     void set(APValue::LValueBase B, unsigned I = 0, bool BInvalid = false) {
+#ifndef NDEBUG
+      // We only allow a few types of invalid bases. Enforce that here.
----------------
Is there a reason this assert only fires in !NDEBUG mode? Is it too expensive to enable for any assert build?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:721
@@ +720,3 @@
+  if (FuncParamNo < 1 || FuncParamNo > FPT->getNumParams()) {
+    auto SrcLoc = Attr.getArgAsExpr(AttrArgNo)->getLocStart();
+    auto UserArgNo = AttrArgNo + 1;
----------------
Please do not use auto unless the type is explicitly spelled out in the initializer (here and elsewhere).

================
Comment at: lib/Sema/SemaDeclAttr.cpp:728
@@ +727,3 @@
+
+  if (!FPT->getParamType(FuncParamNo - 1)->isIntegerType()) {
+    auto SrcLoc = Attr.getArgAsExpr(AttrArgNo)->getLocStart();
----------------
Are character types and Boolean types also okay?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:764
@@ +763,3 @@
+  auto *FD = dyn_cast<FunctionDecl>(D);
+  if (FD == nullptr || !FD->hasPrototype()) {
+    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
----------------
If you use HasFunctionProto in Attr.td, none of this is required. If you can't use HasFunctionProto, then there's still no need to check FD because that's handled by the common attribute code.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:766
@@ +765,3 @@
+    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+        << Attr.getName() << 4;
+    return;
----------------
Please, no magic numbers. I don't think 4 is even the correct value, that's ExpectedParameter.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:777
@@ +776,3 @@
+
+  auto *SizeExpr = Attr.getArgAsExpr(0);
+  int SizeArgNo;
----------------
Please do not use auto unless the type is explicitly spelled out in the initializer.


http://reviews.llvm.org/D14274