[PATCH] D29599: Clang Changes for alloc_align

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter.  The
+alignment parameter is one-indexed.  In the case of member functions, the
----------------
Use single spacing instead of double spacing.


================
Comment at: include/clang/Basic/AttrDocs.td:253
+pointer type) has an alignment specified by the indicated parameter.  The
+alignment parameter is one-indexed.  In the case of member functions, the
+implicit ``this`` parameter is considered as as the first function parameter.
----------------
s/one-index/one-based?

member functions -> nonstatic member functions


================
Comment at: include/clang/Basic/AttrDocs.td:254
+alignment parameter is one-indexed.  In the case of member functions, the
+implicit ``this`` parameter is considered as as the first function parameter.
+
----------------
is considered as as the first -> is the first


================
Comment at: include/clang/Basic/AttrDocs.td:258
+
+  // The returned pointer has the alignment specified by the first parameter
+  void *a(size_t align) __attribute__((alloc_align(1)));
----------------
Missing full stop.


================
Comment at: include/clang/Basic/AttrDocs.td:261
+
+  // The returned pointer has the alignment specified by the second parameter
+  void *b(void* v, size_t align) __attribute__((alloc_align(2)));
----------------
Same here.


================
Comment at: include/clang/Basic/AttrDocs.td:262
+  // The returned pointer has the alignment specified by the second parameter
+  void *b(void* v, size_t align) __attribute__((alloc_align(2)));
+
----------------
Pointer should bind to `v` (same below).


================
Comment at: lib/Sema/SemaDeclAttr.cpp:809
+/// an integral type. Will emit appropriate diagnostics if this returns
+/// false.
+///
----------------
This can be hoisted up to the previous line.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1530
+                                   const AttributeList &Attr) {
+  Expr *E = Attr.getArgAsExpr(0);
+  S.AddAllocAlignAttr(Attr.getRange(), D, E, 
----------------
There's no real need for `E`.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1532
+  S.AddAllocAlignAttr(Attr.getRange(), D, E, 
+                       Attr.getAttributeSpellingListIndex());
+}
----------------
This formatting looks off (the args should align).


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1608-1612
+  IndexVal += 1 + isInstanceMethod(FuncDecl);
+
+  if (!checkParamIsIntegerType(*this, FuncDecl, TmpAttr, ParamExpr, IndexVal,
+                               /*AttrArgNo=*/0, /*AllowDependentType=*/true))
+    return;
----------------
Hmmm, I think this might be a bit more clean as:
```
QualType Ty = getFunctionOrMethodParamType(D, IndexVal);
if (!Ty->isIntegralType(Context)) {
  Diag(ParamExpr->getLocStart(), diag::err_attribute_integers_only) << TmpAttr <<
     FuncDecl->getParamDecl(IndexVal)->getSourceRange();
  return;
}

// We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
// because that has corrected for the implicit this parameter, and is zero-
// based.  The attribute expects what the user wrote explicitly.
llvm::APSInt Val;
ParamExpr->EvaluateAsInt(Val, S.Context);

// Use Val.getZExtValue() when you need what the user wrote.
```
This matches more closely with how other attributes handle the situation where the Attr object needs what the user wrote (like format_arg). I hadn't noticed that checkParamIsIntegerType() turns around and calls checkFunctionOrMethodParameterIndex() again, and this would simplify your patch a bit.

What do you think?


https://reviews.llvm.org/D29599