Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang2il.go: Check for desugared types (support clang-19+) #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rcalixte
Copy link
Contributor

@rcalixte rcalixte commented Jan 3, 2025

@rcalixte
Copy link
Contributor Author

rcalixte commented Jan 3, 2025

I'm working on attaching the full diff with a clean cache now. I'll push it up shortly. In hindsight, I probably should've waited to avoid the build failing. Sorry about that.

@rcalixte
Copy link
Contributor Author

rcalixte commented Jan 4, 2025

Hmm, looking at the diff closer, it seems to remove a few functions. Should those be removed or should we look for re-adding them back in?

They're all in qt6:

  • qbrush
  • qvariantanimation
  • network/qabstractnetworkcache
  • network/qnetworkreply

@mappu
Copy link
Owner

mappu commented Jan 4, 2025

The missing functions do not look very important, we could live without them if it makes clang-19 work.

I think it is fixable -

Here's my (unconfirmed) theory:

Taking qt6 qbrush.h QGradient::Stops as an example, the typedef has changed from

		"type": {
			"desugaredQualType": "QList\u003cstd::pair\u003cdouble, QColor\u003e\u003e",
			"qualType": "QList\u003cQGradientStop\u003e"
		}

With the desugaredQualType, the QGradientStop gets expanded immediately into std::pair, and std::pair is blocklisted 😱 in config-allowlist.go. By comparison, in qt5 this same typedef is present as

		"type": {
			"desugaredQualType": "QVector\u003cQPair\u003cdouble, QColor\u003e\u003e",
			"qualType": "QVector\u003cQGradientStop\u003e"
		}

which is apparently understood better.

In that case, one possible solution would be to remove the block on std::pair. If that's not enough by itself, it may also require handling in intermediate.go QPairOf().

Miqt's comment in config-allowlist.go:318 about QGradientStop is definitely outdated 😄

@rcalixte
Copy link
Contributor Author

rcalixte commented Jan 4, 2025

In that case, one possible solution would be to remove the block on std::pair. If that's not enough by itself, it may also require handling in intermediate.go QPairOf().

Good call. We now have support (however infrequent 😅) for std::pair. It's on my list for after I release what I'm working on to dig into other std:: types that we can bind without too much fuss but this is a good start to tearing down some of those walls I think. Thanks again!

@rcalixte rcalixte force-pushed the desugared_types branch 3 times, most recently from b350bb3 to 7c32809 Compare January 11, 2025 03:11
@mappu
Copy link
Owner

mappu commented Jan 11, 2025

I had another look at this today. The genbindings and std::pair changes all look fine to me 👍 The remaining part of the diff converts many int types (uint64, intptr, ptrdiff) to C types (unsigned long long, etc). This conversion is platform-specific and I think it could fail on Windows where "long" has a different bit size than Linux.

The CI tests win64-qt5 already, and that build passed OK. I updated the CI rules on master, so it will test win32-qt5 and win64-qt6.8 as well.

  • Can you please rebase once more?

If it then passes on all those Windows platforms, then LGTM to merge. But if it doesn't pass we'll need to think more carefully about how int types are handled.

@rcalixte
Copy link
Contributor Author

rcalixte commented Jan 11, 2025

The remaining part of the diff converts many int types (uint64, intptr, ptrdiff) to C types (unsigned long long, etc).

We could always revert these back in emitcabi.go right? I don't mind doing that or reworking the logic for the desugared types. We could skip certain desugared types or just redo the logic entirely. (This would also allow us to re-add that QHashSeed function that was just removed too right?)

@arnetheduck
Copy link
Contributor

We could always revert these back in emitcabi.go right?

Technically the c sources become a little bit more portable if they retain intptr_t etc, ie they better capture the original qt intent - you can't get these back from int, long etc since they are different depending on os/cpu/abi/gcc flags sometimes/etc - above all, if the generated C files are to be used from C, this could be a problem.

@rcalixte
Copy link
Contributor Author

Technically the c sources become a little bit more portable if they retain intptr_t etc, ie they better capture the original qt intent - you can't get these back from int, long etc since they are different depending on os/cpu/abi/gcc flags sometimes/etc - above all, if the generated C files are to be used from C, this could be a problem.

Maybe it's a hack (just wait until you see what I've done 😆), but what if we exclude those types from the desugaring? I'm thinking a conditional for _t types and/or intptr types or something along that course.

@arnetheduck
Copy link
Contributor

I guess one can grab a list from https://pubs.opengroup.org/onlinepubs/009604599/basedefs/stdint.h.html and it would hopefully cover most of it? always a mess with c, finding the right level of de-macro-and-typedef-ification ;) it's not an unreasonable starting point at least.

@rcalixte
Copy link
Contributor Author

rcalixte commented Jan 11, 2025

it's not an unreasonable starting point at least.

I ended up grabbing the Qt typedefs from https://doc.qt.io/qt-6/qttypes.html too. The diff didn't change much without it. The result change in this pull request is now much smaller.

@arnetheduck
Copy link
Contributor

👍

https://pubs.opengroup.org/onlinepubs/7908799/xsh/stddef.h.html is another source, pre-c99 so probably also good to cover

cmd/genbindings/clang2il.go Outdated Show resolved Hide resolved
cmd/genbindings/clang2il.go Outdated Show resolved Hide resolved
@rcalixte rcalixte closed this Jan 20, 2025
@rcalixte rcalixte reopened this Jan 20, 2025
@rcalixte rcalixte force-pushed the desugared_types branch 3 times, most recently from 32b2181 to 33960ee Compare January 20, 2025 19:30
@rcalixte
Copy link
Contributor Author

This managed to get both hackier and cleaner at the same time. 😔

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.

genbindings doesn't work with clang-19
3 participants