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

Fix index types in testfilter #408

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Conversation

atsampson
Copy link
Collaborator

@Gamnn reported that Clang (I tested with LLVM 8.0.1) gives a warning for the first of these, and a narrowing error for the second.

Clang 8 gives a warning for the first of these, and a narrowing error
for the second.
@atsampson atsampson added bug ld-decode-tools An issue only affecting the ld-decode-tools labels Jan 17, 2020
Copy link
Collaborator

@simoninns simoninns left a comment

Choose a reason for hiding this comment

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

Generally it's better to use C++ standard variable types... but there is one big exception... portability. Qt ensures that all Qt types should work cross-platform; that's why I stick to qint32 qreal and the like. Do you think that would be a better change here or am I totally wrong about the portability thing?

@atsampson
Copy link
Collaborator Author

The problem here is that vector::size's return type will depend on the platform's pointer size, but the test code ends up computing a fixed-size sample value from the index, so you have to have a potentially-narrowing conversion somewhere in the middle. (And I used the wrong one!)

We don't normally see this problem with code using Qt's collections like QVector because their size() returns int on all platforms (which IIRC dates back to the early days of Qt 4). The downside of that design is that on modern platforms QVector's total size is artifically limited to < 2GiB, which is a bit of a pain.

I'm generally a bit dubious about the benefits of using qint32 for array indexes in the code (as opposed to using fixed-size types for things that actually are fixed sizes, like sample values). Qt 5 only supports platforms with 32-bit int, and qint32 is in practice always a typedef for int at the moment. And in the unlikely case that a future version of Qt supported platforms with bigger int, we'd then see conversion problems like this in many of the places where we're using QByteArray::size, etc.

(This also made me look at how qreal is defined -- it's not actually intended as a fixed-size floating point type, it's the type used for coordinates within Qt GUIs (QT_COORD_TYPE), and you can override it to be a different type when configuring Qt. So it's the right thing to use if, say, you're computing the position of a slider, but we probably shouldn't be using it for actual maths!)

@simoninns
Copy link
Collaborator

Then, no problems for me; thanks for the explanation :)

@Gamnn
Copy link
Contributor

Gamnn commented Jan 17, 2020

Builds on clang now. It does still throw some warnings about missing braces, (deemp.h has a bunch of these too), but it builds. Thanks!

@atsampson atsampson merged commit 977879f into happycube:master Jan 17, 2020
@atsampson atsampson deleted the narrowingfilter branch January 17, 2020 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ld-decode-tools An issue only affecting the ld-decode-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants