[Clang][ItaniumMangle][NFC] Refactor FunctionTypeDepthState by eiytoq · Pull Request #196240 · llvm/llvm-project

eiytoq · 2026-05-07T05:20:01Z

This patch refactors FunctionTypeDepthState to use bit-fields and moves the
getNestingDepth logic into it. It also renames {enter,leave}ResultType to
{enter,leave}FunctionDeclSuffix, since the old names no longer match their
current role.

llvmorg-github-actions · 2026-05-07T05:20:51Z

@llvm/pr-subscribers-clang

Author: eiytoq (eiytoq)

Changes

This patch refactors FunctionTypeDepthState to use bit-fields and moves the
getNestingDepth logic into it. It also renames {enter,leave}ResultType to
{enter,leave}FunctionDeclSuffix, since the old names no longer match their
current role.

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

1 Files Affected:

(modified) clang/lib/AST/ItaniumMangle.cpp (+29-43)

diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index f58faa03bfa8c..d07b4b38a0950 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -243,42 +243,34 @@ class CXXNameMangler {
   unsigned SeqID = 0;
 
   class FunctionTypeDepthState {
-    unsigned Bits = 0;
-
-    enum { InResultTypeMask = 1 };
+    unsigned Depth : 31;
+    unsigned InFunctionDeclSuffix : 1;
 
   public:
-    FunctionTypeDepthState() = default;
-
-    /// The number of function types we're inside.
-    unsigned getDepth() const {
-      return Bits >> 1;
-    }
-
-    /// True if we're in the return type of the innermost function type.
-    bool isInResultType() const {
-      return Bits & InResultTypeMask;
+    FunctionTypeDepthState() : Depth(0), InFunctionDeclSuffix(0) {}
+
+    unsigned getNestingDepth(unsigned ParmDepth) const {
+      // ParmDepth does not include the declaring function prototype.
+      // FunctionTypeDepth does account for that.
+      assert(ParmDepth < Depth &&
+             "ParmVarDecl is not visible in current parameter environment");
+      return Depth - ParmDepth - InFunctionDeclSuffix;
     }
 
     FunctionTypeDepthState push() {
-      FunctionTypeDepthState tmp = *this;
-      Bits = (Bits & ~InResultTypeMask) + 2;
-      return tmp;
-    }
-
-    void enterResultType() {
-      Bits |= InResultTypeMask;
-    }
-
-    void leaveResultType() {
-      Bits &= ~InResultTypeMask;
+      FunctionTypeDepthState Saved = *this;
+      ++Depth;
+      InFunctionDeclSuffix = 0;
+      return Saved;
     }
 
-    void pop(FunctionTypeDepthState saved) {
-      assert(getDepth() == saved.getDepth() + 1);
-      Bits = saved.Bits;
+    void pop(FunctionTypeDepthState Saved) {
+      assert(Depth == Saved.Depth + 1 && "unbalanced function type depth pop");
+      *this = Saved;
     }
 
+    void enterFunctionDeclSuffix() { InFunctionDeclSuffix = 1; }
+    void leaveFunctionDeclSuffix() { InFunctionDeclSuffix = 0; }
   } FunctionTypeDepth;
 
   // abi_tag is a gcc attribute, taking one or more strings called "tags".
@@ -3752,9 +3744,9 @@ void CXXNameMangler::mangleType(const FunctionNoProtoType *T) {
 
   FunctionTypeDepthState saved = FunctionTypeDepth.push();
 
-  FunctionTypeDepth.enterResultType();
+  FunctionTypeDepth.enterFunctionDeclSuffix();
   mangleType(T->getReturnType());
-  FunctionTypeDepth.leaveResultType();
+  FunctionTypeDepth.leaveFunctionDeclSuffix();
 
   FunctionTypeDepth.pop(saved);
   Out << 'E';
@@ -3769,7 +3761,7 @@ void CXXNameMangler::mangleBareFunctionType(const FunctionProtoType *Proto,
 
   // <bare-function-type> ::= <signature type>+
   if (MangleReturnType) {
-    FunctionTypeDepth.enterResultType();
+    FunctionTypeDepth.enterFunctionDeclSuffix();
 
     // Mangle ns_returns_retained as an order-sensitive qualifier here.
     if (Proto->getExtInfo().getProducesResult() && FD == nullptr)
@@ -3784,7 +3776,7 @@ void CXXNameMangler::mangleBareFunctionType(const FunctionProtoType *Proto,
     }
     mangleType(ReturnTy);
 
-    FunctionTypeDepth.leaveResultType();
+    FunctionTypeDepth.leaveFunctionDeclSuffix();
   }
 
   if (Proto->getNumParams() == 0 && !Proto->isVariadic()) {
@@ -3821,7 +3813,7 @@ void CXXNameMangler::mangleBareFunctionType(const FunctionProtoType *Proto,
   }
 
   if (FD) {
-    FunctionTypeDepth.enterResultType();
+    FunctionTypeDepth.enterFunctionDeclSuffix();
     mangleRequiresClause(FD->getTrailingRequiresClause().ConstraintExpr);
   }
 
@@ -5716,7 +5708,7 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity,
       Out << '_';
 
       // The rest of the mangling is in the immediate scope of the parameters.
-      FunctionTypeDepth.enterResultType();
+      FunctionTypeDepth.enterFunctionDeclSuffix();
       for (const concepts::Requirement *Req : RE->getRequirements())
         mangleRequirement(RE->getExprLoc(), Req);
       FunctionTypeDepth.pop(saved);
@@ -6013,14 +6005,8 @@ void CXXNameMangler::mangleFunctionParam(const ParmVarDecl *parm) {
   unsigned parmIndex = parm->getFunctionScopeIndex();
 
   // Compute 'L'.
-  // parmDepth does not include the declaring function prototype.
-  // FunctionTypeDepth does account for that.
-  assert(parmDepth < FunctionTypeDepth.getDepth());
-  unsigned nestingDepth = FunctionTypeDepth.getDepth() - parmDepth;
-  if (FunctionTypeDepth.isInResultType())
-    nestingDepth--;
-
-  if (nestingDepth == 0) {
+  if (unsigned nestingDepth = FunctionTypeDepth.getNestingDepth(parmDepth);
+      nestingDepth == 0) {
     Out << "fp";
   } else {
     Out << "fL" << (nestingDepth - 1) << 'p';
@@ -7254,9 +7240,9 @@ CXXNameMangler::makeFunctionReturnTypeTags(const FunctionDecl *FD) {
   const FunctionProtoType *Proto =
       cast<FunctionProtoType>(FD->getType()->getAs<FunctionType>());
   FunctionTypeDepthState saved = TrackReturnTypeTags.FunctionTypeDepth.push();
-  TrackReturnTypeTags.FunctionTypeDepth.enterResultType();
+  TrackReturnTypeTags.FunctionTypeDepth.enterFunctionDeclSuffix();
   TrackReturnTypeTags.mangleType(Proto->getReturnType());
-  TrackReturnTypeTags.FunctionTypeDepth.leaveResultType();
+  TrackReturnTypeTags.FunctionTypeDepth.leaveFunctionDeclSuffix();
   TrackReturnTypeTags.FunctionTypeDepth.pop(saved);
 
   return TrackReturnTypeTags.AbiTagsRoot.getSortedUniqueUsedAbiTags();

ojhunt

This looks like a good improvement to me - it took me a while to go through the cursed bit manipulation, but the logic all seems correct to me.

I don't know about the renaming - I'm not sufficiently familiar with mangling terminology so I'd like @erichkeane or @shafik to ok that separately

eiytoq · 2026-05-14T08:35:59Z

Currently, the flag indicates whether the mangler is processing the function declaration suffix(e.g. return type or trailing requires-clause), so we could rename it to match its role.

eiytoq · 2026-05-16T09:55:42Z

If there are no further comments, I plan to merge this PR.

erichkeane · 2026-05-17T22:58:50Z

If there are no further comments, I plan to merge this PR.

I'll note that this is absolutely inappropriate to do here. You had an approver who said to wait for other reviewers, and a reviewer with questions that you ignored. Additionally, you gave only 1 'business' day of warning before committing here.

THis sort of "speak now or I'll merge it" type of comment is completely inappropriate for this project, and is a great way to lose your ability to contribute to clang. @AaronBallman

eiytoq · 2026-05-18T00:49:28Z

@erichkeane This was my mistake. I treated silence as approval and landed this too quickly and I didn't account for the weekend. I can revert the PR if you think that's best.

Do you have any thoughts on the renaming?

erichkeane · 2026-05-18T02:43:12Z

@erichkeane This was my mistake. I treated silence as approval and landed this too quickly and I didn't account for the weekend. I can revert the PR if you think that's best.

Do you have any thoughts on the renaming?

We don't take silence as approval, we need positive approval from folks. As far as the rename: I don't think I feel strongly here, but perhaps our maintainers of this (@efriedma-quic ?) might have an idea.

As the questions/concerns are minor so far, I think I'm OK with us holding off on reverting unless someone spots one that needs it. I WOULD like @shafik to make sure he's happy though, since he had a number of questions that I tried to answer but we should make sure he was satisfied by them.

AaronBallman · 2026-05-18T13:11:26Z

+    void enterFunctionDeclSuffix() { InFunctionDeclSuffix = 1; }
+    void leaveFunctionDeclSuffix() { InFunctionDeclSuffix = 0; }


Should we guard against mismatched calls? e.g., assert that we cannot leave if we've not entered, and assert that we cannot enter if we're already entered?

I think this makes sense. Because every time FunctionTypeDepth.push() is called, InFunctionDeclSuffix is reset, and at the same depth, enterFunctionDeclSuffix should not be entered repeatedly. However, I'm not entirely sure if I've missed any details.

AaronBallman · 2026-05-18T13:15:03Z

    mangleType(ReturnTy);

-    FunctionTypeDepth.leaveResultType();
+    FunctionTypeDepth.leaveFunctionDeclSuffix();


Would it make sense to add an RAII object to do the enter/leave calls? I worry about the possibility of adding a return in here someday that breaks this.

We could introduce an RAII object (because the missing leave below is actually omitted), but that might require guarding every call site.

AaronBallman · 2026-05-18T13:15:46Z


  if (FD) {
-    FunctionTypeDepth.enterResultType();
+    FunctionTypeDepth.enterFunctionDeclSuffix();


Ohhh, RAII object might not make sense, what ends up leaving this?

The state will be restored by the following FunctionTypeDepth.pop(saved), so leaveFunctionDeclSuffix can be omitted here.

AaronBallman · 2026-05-18T13:16:29Z


      // The rest of the mangling is in the immediate scope of the parameters.
-      FunctionTypeDepth.enterResultType();
+      FunctionTypeDepth.enterFunctionDeclSuffix();


What ends up leaving this?

Same as above.

AaronBallman · 2026-05-18T13:17:32Z

@erichkeane This was my mistake. I treated silence as approval and landed this too quickly and I didn't account for the weekend. I can revert the PR if you think that's best.
Do you have any thoughts on the renaming?

We don't take silence as approval, we need positive approval from folks. As far as the rename: I don't think I feel strongly here, but perhaps our maintainers of this (@efriedma-quic ?) might have an idea.

As the questions/concerns are minor so far, I think I'm OK with us holding off on reverting unless someone spots one that needs it. I WOULD like @shafik to make sure he's happy though, since he had a number of questions that I tried to answer but we should make sure he was satisfied by them.

Thanks for catching this @erichkeane! I don't think we need a revert yet, but I also had some questions on the PR.

) This patch refactors `FunctionTypeDepthState` to use bit-fields and moves the `getNestingDepth` logic into it. It also renames `{enter,leave}ResultType` to `{enter,leave}FunctionDeclSuffix`, since the old names no longer match their current role.

shafik

I got my answers but it looks like Aaron has more.

llvmorg-github-actions Bot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 7, 2026

eiytoq force-pushed the nfc/clang-function-depth branch from aef4610 to 3a82d0c Compare May 7, 2026 05:24

[Clang][ItaniumMangle][NFC] Refactor FunctionTypeDepthState

dfea8e8

eiytoq force-pushed the nfc/clang-function-depth branch from 3a82d0c to dfea8e8 Compare May 8, 2026 17:02

shafik reviewed May 13, 2026

View reviewed changes

Comment thread clang/lib/AST/ItaniumMangle.cpp

Comment thread clang/lib/AST/ItaniumMangle.cpp

shafik requested review from erichkeane and ojhunt May 13, 2026 18:22

ojhunt approved these changes May 14, 2026

View reviewed changes

eiytoq merged commit a264335 into llvm:main May 17, 2026
10 checks passed

AaronBallman reviewed May 18, 2026

View reviewed changes

shafik reviewed May 20, 2026

View reviewed changes

		void enterFunctionDeclSuffix() { InFunctionDeclSuffix = 1; }
		void leaveFunctionDeclSuffix() { InFunctionDeclSuffix = 0; }

Uh oh!

Conversation

eiytoq commented May 7, 2026

Uh oh!

llvmorg-github-actions Bot commented May 7, 2026

Uh oh!

Uh oh!

Uh oh!

ojhunt left a comment

Choose a reason for hiding this comment

Uh oh!

eiytoq commented May 14, 2026

Uh oh!

eiytoq commented May 16, 2026

Uh oh!

Uh oh!

erichkeane commented May 17, 2026

Uh oh!

eiytoq commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented May 18, 2026

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eiytoq commented May 18, 2026 •

edited

Loading