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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits to be fixed.



================
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 {
----------------
aaron.ballman wrote:
> No `else` after a `return`.
You can still drop this `else` (and use a regular `if`) because of the preceding `return`.


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


================
Comment at: clang/lib/Sema/SemaChecking.cpp:246-259
+    if (AlignValue < 1) {
+      S.Diag(AlignOp->getExprLoc(), diag::err_alignment_too_small) << 1;
+      return true;
+    } else if (llvm::APSInt::compareValues(AlignValue, MaxValue) > 0) {
+      S.Diag(AlignOp->getExprLoc(), diag::err_alignment_too_big)
+          << MaxValue.toString(10);
+      return true;
----------------
There are a bunch of `else` after `return` issues in this ladder. I would rearrange to put the error cases first and the warning case last.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71499