Refactor ASTContext::getDeclAlign() (NFC)

[JonPsson1]

@efriedma-quic
@rjmccall

While working on this (with the other patch: 72886), I found this refactoring at the top of the function which I like. What do you think?

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jonas Paulsson (JonPsson1)

Changes

@efriedma-quic
@rjmccall

While working on this (with the other patch: 72886), I found this refactoring at the top of the function which I like. What do you think?

---

Full diff: https://github.com/llvm/llvm-project/pull/72977.diff

1 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+13-20)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 1c893d008cb49f3..d08b9072c0e6298 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1627,28 +1627,21 @@ const llvm::fltSemantics &ASTContext::getFloatTypeSemantics(QualType T) const {
 CharUnits ASTContext::getDeclAlign(const Decl *D, bool ForAlignof) const {
   unsigned Align = Target->getCharWidth();
 
-  bool UseAlignAttrOnly = false;
-  if (unsigned AlignFromAttr = D->getMaxAlignment()) {
+  const unsigned AlignFromAttr = D->getMaxAlignment();
+  if (AlignFromAttr)
     Align = AlignFromAttr;
 
-    // __attribute__((aligned)) can increase or decrease alignment
-    // *except* on a struct or struct member, where it only increases
-    // alignment unless 'packed' is also specified.
-    //
-    // It is an error for alignas to decrease alignment, so we can
-    // ignore that possibility;  Sema should diagnose it.
-    if (isa<FieldDecl>(D)) {
-      UseAlignAttrOnly = D->hasAttr<PackedAttr>() ||
-        cast<FieldDecl>(D)->getParent()->hasAttr<PackedAttr>();
-    } else {
-      UseAlignAttrOnly = true;
-    }
-  }
-  else if (isa<FieldDecl>(D))
-      UseAlignAttrOnly =
-        D->hasAttr<PackedAttr>() ||
-        cast<FieldDecl>(D)->getParent()->hasAttr<PackedAttr>();
-
+  // __attribute__((aligned)) can increase or decrease alignment
+  // *except* on a struct or struct member, where it only increases
+  // alignment unless 'packed' is also specified.
+  //
+  // It is an error for alignas to decrease alignment, so we can
+  // ignore that possibility;  Sema should diagnose it.
+  bool IsPackedField = isa<FieldDecl>(D) &&
+                       (D->hasAttr<PackedAttr>() ||
+                        cast<FieldDecl>(D)->getParent()->hasAttr<PackedAttr>());
+  bool UseAlignAttrOnly =
+    isa<FieldDecl>(D) ? IsPackedField : AlignFromAttr;
   // If we're using the align attribute only, just ignore everything
   // else about the declaration and its type.
   if (UseAlignAttrOnly) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[efriedma-quic]

That's a lot of isa<> and cast<>. I think I'd prefer something more like:

bool UseAlignAttrOnly;
if (FieldDecl *FD = dyn_cast<FieldDecl>(D)) {
  UseAlignAttrOnly = FD->hasAttr<PackedAttr>() ||
                     FD->getParent()->hasAttr<PackedAttr>();
} else {
  UseAlignAttrOnly = AlignFromAttr != 0;
}

[JonPsson1]

That works for me as well: main problem to me with the original code was the duplicated handling of a FieldDecl.

I took your suggestion, but removed the braces, which seem superfluous? Or are they required by the coding standard?

[JonPsson1]

Eli, are you supposed to press some button before I merge this?

[efriedma-quic]

LGTM

See https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements for the rules on use/don't use braces. A single multi-line statement is sort of on the edge; I tend to prefer braces in that case, but you can go either way.