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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
