Skip to content

[cling] Desugar UsingType in GetFullyQualifiedType#22742

Closed
devajithvs wants to merge 2 commits into
root-project:masterfrom
devajithvs:usingtype_desugaring
Closed

[cling] Desugar UsingType in GetFullyQualifiedType#22742
devajithvs wants to merge 2 commits into
root-project:masterfrom
devajithvs:usingtype_desugaring

Conversation

@devajithvs

@devajithvs devajithvs commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

The change in getFullyQualifiedType() in the commit llvm/llvm-project@af27466 added explicit desugaring of UsingType:

  if (isa<UsingType>(QT.getTypePtr())) {
    return getFullyQualifiedType(QT.getSingleStepDesugaredType(Ctx), Ctx,
                                 WithGlobalNsPrefix);
  }

This has been there since LLVM14 and we could disentagle this from the upgrade to LLVM22.

This will cause SG::sgkey_t to be resolved to its underlying type uint32_t when GetFullTypeName() is called, rather than preserving the typedef alias name as before.

This test was added in: #19615, should fix: #12698

This will be the default behaviour (in LLVM22) after #21658. So decoupling this change from the upgrade here.

This Pull request:

Changes or fixes:

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@devajithvs
devajithvs requested review from dpiparo and hahnjo as code owners July 2, 2026 08:00
@devajithvs devajithvs self-assigned this Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 15h 3m 5s ⏱️
 3 874 tests  3 851 ✅ 0 💤 23 ❌
78 807 runs  78 784 ✅ 0 💤 23 ❌

For more details on these failures, see this check.

Results for commit 0abc723.

♻️ This comment has been updated with latest results.

@devajithvs
devajithvs requested a review from vgvassilev July 3, 2026 14:40
@vgvassilev
vgvassilev requested a review from pcanal July 3, 2026 16:52
@vgvassilev

Copy link
Copy Markdown
Member

Is this is what we really want? I am not sure if we should be losing that type sugar but @pcanal should be able to tell...

@devajithvs

Copy link
Copy Markdown
Contributor Author

This is what upstream does. We maintain a separate GetFullyQualifiedType implementation anyway. I was trying to reduce the divergence with the upgrade.

@vgvassilev

Copy link
Copy Markdown
Member

This is what upstream does. We maintain a separate GetFullyQualifiedType implementation anyway. I was trying to reduce the divergence with the upgrade.

Yes, I agree with that, let's see what @pcanal has to say if that's not breaking something in i/o conceptually.

@pcanal pcanal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@hahnjo

hahnjo commented Jul 13, 2026

Copy link
Copy Markdown
Member

To mirror what I wrote on Mattermost, after looking at the test change, I'm not so sure anymore: SG::sgkey_t is the full type name of data member m_sgkey. It aliases to uint32_t, which then aliases to unsigned int

The change in getFullyQualifiedType() in the commit
llvm/llvm-project@af27466 added explicit
desugaring of UsingType:

```
  if (isa<UsingType>(QT.getTypePtr())) {
    return getFullyQualifiedType(QT.getSingleStepDesugaredType(Ctx), Ctx,
                                 WithGlobalNsPrefix);
  }
```

This has been there since LLVM14 and we could disentagle this from the
upgrade to LLVM22.

This will cause `SG::sgkey_t` to be resolved to its underlying type
`uint32_t` when GetFullTypeName() is called, rather than preserving
the typedef alias name as before.
@devajithvs
devajithvs force-pushed the usingtype_desugaring branch from 0abc723 to 4ba84a6 Compare July 13, 2026 13:24
@devajithvs

Copy link
Copy Markdown
Contributor Author

Yes, we should probably get the canonical type instead of just going one layer down.

UT->getUnderlyingType() which only strips one sugar layer, stopping at the
first TypedefType rather than reaching the canonical type.

Use QT.getCanonicalType() instead to strip all sugar layers at once.
@devajithvs
devajithvs force-pushed the usingtype_desugaring branch from 4ba84a6 to 281f335 Compare July 13, 2026 13:26
@vgvassilev

Copy link
Copy Markdown
Member

Yes, but this is using then the wrong facility..

@devajithvs

Copy link
Copy Markdown
Contributor Author

I see, thanks for the pointer. I understand now that GetFullTypeName() should preserve the alias spelling as it was written.

I will close this PR, keep the existing m_sgkey test as-is, and add a separate test case for UsingType (same behavior as TypedefType) in LLVM22 PR.

The problem here was that, even though top-level node here is a TypedefType:

TypedefType 0x5bca02c32f60 'SG::sgkey_t' sugar
  ...
  UsingType 0x5bca02c32490 'std::uint32_t' sugar
  ...
    UsingShadow 0x5bca02c32280 'uint32_t'

getAs<UsingType>() traverses through the type sugar until it finds the underlying UsingType node, here returns the UsingType declaration for std::uint32_t.

@devajithvs devajithvs closed this Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[core] Difference in typedef/using resolution in Windows and Linux builds

4 participants