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

Q_ENUMS cannot use LLONG_MAX #5

Closed
wants to merge 1 commit into from
Closed

Conversation

jmlich
Copy link
Contributor

@jmlich jmlich commented Dec 8, 2023

The Q_ENUMS is deprecated and it is advised to use Q_ENUM see https://doc.qt.io/qt-6/qobject-obsolete.html#Q_ENUMS

The Q_ENUM explicitly says that enum is stored as signed int. See https://doc.qt.io/qt-6/qobject.html#Q_ENUM

In the end the InvalidValue64 = LLONG_MAX assignment causes that whole enum type long long, which is not allowed in Qt. I was thinking that this is probably Qt bug, but it is actually feature https://bugreports.qt.io/browse/QTBUG-119620

It leads to strange compilation error in Qt 6.6 (maybe even 6.5). See attached build.log. I am not sure if this solution is good or wise, but I hope it will work until we will find better solution.

@pvuorela
Copy link
Contributor

pvuorela commented Dec 8, 2023

I wonder why such a special 64-bit enum value even exists. Not spotting even usage immediately.

@mlehtima your addition, maybe you have some idea. Could we just use one enum with same value everywhere?

Addition: wonder how such should an enum value even be used. "if (someValue == Invalid || someValue == Invalid64)" or ifdeffing based on bitness? Both would feel ugly.

@monich
Copy link
Member

monich commented Dec 9, 2023

how about this:

qofonoextcell.h

    static const int InvalidValue;
    static const qint64 InvalidValue64;

qofonoextcell.cpp

const int QOfonoExtCell::InvalidValue = INT_MAX;
const qint64 QOfonoExtCell::InvalidValue64 = LLONG_MAX;

@pvuorela
Copy link
Contributor

how about this:
[...]

Maybe that's slightly better but still seems unusable for the client of this api. Those shouldn't need to care about the bitness of the system.

I'd say either

  • Kill InvalidValue64 somehow and define just one invalid value, or
  • Where applicable here, replace the property types int -> QVariant and use invalid variant for the "doesn't exist" case.

@monich
Copy link
Member

monich commented Dec 12, 2023

Ah, forget what I proposed, those constants are exposed to QML, they have to remain what they are, to avoid breaking the API.

@pvuorela
Copy link
Contributor

Ah, forget what I proposed, those constants are exposed to QML, they have to remain what they are, to avoid breaking the API.

Still repeating what I've been saying, don't expect anyone really using Invalid64 from QML as such bitness cannot even use c/c++ ifdes. Code would need to always compare if it's either of the invalids. That and this being an non-public component and Invalid64 never included in a sailfish release so far. At least that has to go.

@pvuorela
Copy link
Contributor

Correcting myself: on closer look the Invalid64 doesn't seem to be about bitness of the system but bitness of the property, i.e. nci being qint64. But that's still not too good api since developer would need to pay close attention on how invalidness of each value is checked.

For another thing, not immediately sure how well that property works in qml as 64 bit properties have been problematic there. Didn't test.

Haven't spotted usage for that either so far.

@mlehtima
Copy link
Contributor

As mentioned by @pvuorela that enum has never reached any Sailfish OS release and it is highly unlikely that anyone would have used the enum so I think it would be fine to remove the broken unusable code.

@pvuorela
Copy link
Contributor

Switched the type to string to allow qml usage.

And as commented on the other PR, noticed while playing that none of the properties work nice in QML as the values cannot be compared directly to invalid value due to javascript float numbers.

@pvuorela pvuorela closed this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants