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

Come up with a forwards-compatible system for name mangling #188

Open
arnetheduck opened this issue Apr 2, 2025 · 5 comments
Open

Come up with a forwards-compatible system for name mangling #188

arnetheduck opened this issue Apr 2, 2025 · 5 comments
Labels

Comments

@arnetheduck
Copy link
Contributor

The current name mangling scheme for disambiguating overloads simple appends a number to the method name (New2, New3 etc).

While simple, it has a number of problems:

  • when calling the function, it's hard to tell which is which - in particular, with types such as QVariant that have overrides for "similar" types, one might pick the wrong number by accident via features like integer size promotion (ie calling QVariant_newXX with a short might compile with the int override as well)
  • between qt5/6 but also in minor feature releases, new overloads may be added - these cause renumbering if added in the middle - again, QVariant is a good example where the QVariant_new6 takes a QDataStream or long long depending on the version of qt
  • adding support for new types such as QVariantList again causes renumbering, as can be seen in the linked code where 22 and onwards get shifted by the new overload

Two potential ways out:

  • parameter names - ie QVariant* QVariant_new4(int i); becomes QVariant* QVariant_new_i(int i); - this is pleasant to read but runs the risk of further conflicts and remains semantically unstable since variable names don't matter in C++ overload resolution rules and therefore might change as part of "cleanups"
  • type names - QVariant* QVariant_new4(int i); becomes QVariant_new_int - the more correct approach, though slightly ugly for pointers, references and consts which have to be mangled as well
    • probably most such qualifiers can simply be stripped, except for cases where two types would collapse to the same name such as overloads for both const and non-const or */&
@mappu mappu added the wishlist label Apr 4, 2025
@mappu
Copy link
Owner

mappu commented Apr 4, 2025

Two potential ways out: parameter names ... type names

There is a limited implementation of this already for function overloads (see transformoverload.go).

But when writing applications, I've found these overloads actually less ergonomic than the numbered versions. The longer names really make the code appear to deviate more from Qt C++ which makes porting more difficult. With numbers, it's less clear to read, but it's easy to write; usually you hit the autocomplete shortcut in your IDE, scroll to the one with the right-looking parameters, and move on with the project -

Another idea to affix the stability (without addressing usability at all) would be to emit the ordering rules as some sort of file format (xml/json) and commit that for future runs of the generator. Then later blacklisting/new function additions/... could preserve the existing ordering as a matter of configuration.

@mappu mappu changed the title Unstable name mangling Come up with a forwards-compatible system for name mangling Apr 4, 2025
@arnetheduck
Copy link
Contributor Author

There is a limited implementation

Yeah, I was wondering about that because I saw it was applied sometimes but not everywhere - I think for the C names at least, removing With and underscoring instead, ie QVariant_new_int, QVariant_new_char results in a style which is common (well, in C at least) and not too hard to type while being pleasant to read, both with and without fancy tooling/autocompletes - but for some reason it's not transforming the QVariant overloads - maybe because they're constructors?

Readability aside, I agree stability is the greater issue here indeed which is why I'm leaning towards the type-based solution as it's guaranteed to do the right thing now and in the future - making the generator stateful, hm, feels fragile, specially if you consider the qt5/6 boundary and a future qt7 that undoubtedly will appear. It can probably handle minor releases just fine though.

Going that down that route, one candidate approach is to parse back the generated C files which should have enough information except for the cases where decay to the same generator types leads to a conflict (ie const char* / QString -> miqt_string).

@rcalixte
Copy link
Contributor

rcalixte commented Apr 4, 2025

I'd actually contemplated removing all of the overloads but haven't touched them yet, largely because of the documentation URLs I'd amended since I think it should help reduce if not remove the potential for confusion. I also find it easier to keep the API as comparable to the Go bindings as possible, especially when it comes to writing the examples. 😅

That said, I'd seen the QVariantList changes and considered implementing them right away, but the way I see it, unless it's needed right away, it could probably wait for a new base version of the bindings like 6.8. I've done more breaking changes already than I would have liked and a new version seems like the right time to introduce these kinds of changes. From what I've observed from 6.4 to 6.8 so far, there are functions that have had their return types modified so a modified integer value in a function here or there is hopefully just as reasonable to navigate during that time. Maybe it would merit a callout in the binding release notes.

I did spend some time thinking about what would be better (if at all) than the current numbering structure for overloading but the feedback that I've gotten so far is that it's easy to grok for developers. My findings were the same when I started playing with the Go bindings myself. Now that they're public, being thoughtful about when to introduce the changes is a burden but not a large one. I've got my notes. 😅

@mappu
Copy link
Owner

mappu commented Apr 5, 2025

Another quick solution (specifically for QVariantList) would be to sort the ctors in transformctors.go so that anything containing the newly exposed type goes to the end, ensuring a new number. There is already custom sorting here for miqt-uic's use.

@arnetheduck
Copy link
Contributor Author

Just for the record, I used QVariantList as an example, but the same thing happens for void*, ByteArrayView and so on - I didn't mean to single it out as being special in any significant regard ;)

More than that though, I think Qt has a good compatibility strategy here that qt devs have come to expect where between minor versions, source and binary compat is kept, and between major releases source compat is mostly kept - very valuable, based on our experience porting a large:ish app from qt5 to 6 using seaqt which has been mostly pain free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants