[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:2844
+The builtins ``__builtin_align_up``, ``__builtin_align_down``, return their
+first argument aligned up/down to the next multiple of the second argument.
+The builtin ``__builtin_is_aligned`` returns whether the first argument is
----------------
Should explicitly specify whether the arguments are expressed in bytes or bits.


================
Comment at: clang/docs/LanguageExtensions.rst:2853
+
+If Clang can determined that the alignment is be not a power of two at
+compile-time, it will result in a compilation failure. If the alignment argument
----------------
determined -> determine
is be not -> is not


================
Comment at: clang/lib/AST/ExprConstant.cpp:8154
+static CharUnits getBaseAlignment(EvalInfo &Info, const LValue &Value) {
+  if (const ValueDecl *VD = Value.Base.dyn_cast<const ValueDecl *>()) {
+    return Info.Ctx.getDeclAlign(VD);
----------------
Can go with `const auto *` where the type is spelled out in the initialization like this (here and elsewhere).


================
Comment at: clang/lib/AST/ExprConstant.cpp:8156-8158
+  } else if (const Expr *E = Value.Base.dyn_cast<const Expr *>()) {
+    return GetAlignOfExpr(Info, E, UETT_AlignOf);
+  } else {
----------------
No `else` after a `return`.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8173
+  }
+  const unsigned SrcWidth = Info.Ctx.getIntWidth(ForType);
+  APSInt MaxValue(APInt::getOneBitSet(SrcWidth, SrcWidth - 1));
----------------
Should drop the top-level `const` qualifier as that isn't an idiom we typically use for locals.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10685
+    if (Src.isLValue()) {
+      // If we evaluated a pointer, check the minimum known alignment
+      LValue Ptr;
----------------
Comment missing a full stop at the end.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10720
+      return Error(E);
+    assert(Src.isInt() && "Adjusting pointer alignment in IntExprEvaluator?");
+    APSInt AlignedVal =
----------------
This assertion doesn't add much given the above line of code.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10734
+      return Error(E);
+    assert(Src.isInt() && "Adjusting pointer alignment in IntExprEvaluator?");
+    APSInt AlignedVal =
----------------
Same here as above.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14267
+    QualType AstType = E->getArg(0)->getType();
+    if (AstType->isArrayType()) {
+      Src = CGF.EmitArrayToPointerDecay(E->getArg(0)).getPointer();
----------------
Can elide braces.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:224
+      SrcTy->isFunctionPointerType()) {
+    // XXX: this is not quite the right error message since we don't allow
+    // floating point types, or member pointers
----------------
Comment should probably be a FIXME unless you intend to address it as part of this patch. Also, the comment is missing a full stop (please check all comments in the patch as many of them are missing one).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71499/new/

https://reviews.llvm.org/D71499