[PATCH] D29599: Clang Changes for alloc_align

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 9 inline comments as done.
erichkeane added inline comments.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1608-1612
+  IndexVal += 1 + isInstanceMethod(FuncDecl);
+
+  if (!checkParamIsIntegerType(*this, FuncDecl, TmpAttr, ParamExpr, IndexVal,
+                               /*AttrArgNo=*/0, /*AllowDependentType=*/true))
+    return;
----------------
aaron.ballman wrote:
> 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?
Looks better, I hadn't realized checkParamIsIntegerType was redoing work either.

I had to make a pair of small changes to this (including omitting DependentType from the integral type check), but it otherwise works nicely.  Thanks!


https://reviews.llvm.org/D29599