Skip to content

Duration util test#2015

Merged
uklotzde merged 3 commits into
mixxxdj:masterfrom
daschuer:DurationUtilTest
Feb 14, 2019
Merged

Duration util test#2015
uklotzde merged 3 commits into
mixxxdj:masterfrom
daschuer:DurationUtilTest

Conversation

@daschuer
Copy link
Copy Markdown
Member

Comment thread src/test/durationutiltest.cpp Outdated
formatKiloSeconds(QString::fromUtf8("0.999\u2009990"), 999.99);
formatKiloSeconds(QString::fromUtf8("1.000\u2009000"), 1000.00);
formatKiloSeconds(QString::fromUtf8("86.400\u2009000"), 24 * 3600);
formatKiloSeconds(QStringLiteral("0.000\u2009000"), 0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the characters in the C++ string literal are UTF-8 encoded shouldn't it be prefixed with u8?

QStringLiteral(u8"0.000\u2009000")

https://en.cppreference.com/w/cpp/language/string_literal

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That might be another solution together with the fromUtf8() call.
The issue was that msvc has replaced the thin space with a replacement ?
Maybe because it uses a ASCII intermediate string or thomething.

QStringLiteral is a macro including the L prefix so it should be save here.

Unfortunately Appveyor has timed out do we have no results on the critical Os. :-(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, the unicode character code should be interpreted correctly no matter which encoding you choose for the string literal.

@daschuer
Copy link
Copy Markdown
Member Author

Since we have no ci for Windows here. Can we merge it and fix reaming issues in a follow up PR if required?

@daschuer
Copy link
Copy Markdown
Member Author

force push to trigger CI

@uklotzde
Copy link
Copy Markdown
Contributor

LGTM, waiting for the CI builds.

@uklotzde
Copy link
Copy Markdown
Contributor

Still doesn't work!?

@daschuer
Copy link
Copy Markdown
Member Author

Appveyor timed out.

So I have tested the solution with msvc 2017 seperately and it works.
Even if it fails it will be no regression.
So let's merge this now.

@uklotzde
Copy link
Copy Markdown
Contributor

Accepted ;)

@uklotzde uklotzde merged commit 0821085 into mixxxdj:master Feb 14, 2019
@daschuer daschuer deleted the DurationUtilTest branch September 7, 2021 21:09
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.

2 participants