Skip to content

Commit

Permalink
remove /Zc:wchar_t- flag and don't use QString::fromWCharArray on Win…
Browse files Browse the repository at this point in the history 
…dows
  • Loading branch information
@daschuer
daschuer committed Oct 12, 2015
1 parent 612a216 commit 344ae07
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 19 deletions.
5 changes: 0 additions & 5 deletions build/depends.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1066,11 +1066,6 @@ def configure(self, build, conf):
else:
raise Exception('Invalid machine type for Windows build.')

# Ugh, MSVC-only hack :( see
# http://www.qtforum.org/article/17883/problem-using-qstring-
# fromstdwstring.html
build.env.Append(CXXFLAGS='/Zc:wchar_t-')

# Build with multiple processes. TODO(XXX) make this configurable.
# http://msdn.microsoft.com/en-us/library/bb385193.aspx
build.env.Append(CCFLAGS='/MP')
Expand Down
27 changes: 13 additions & 14 deletions src/controllers/hid/hidcontroller.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,18 @@ void HidReader::run() {
delete [] data;
}

QString safeDecodeWideString(wchar_t* pStr, size_t max_length) {
if (pStr == NULL) {
return QString();
QString safeDecodeWideString(const wchar_t* pStr, size_t max_length) {
#ifdef __WINDOWS__
// We cannot use Qts wchar_t functions, since the may work or not
// depending on the '/Zc:wchar_t-' build flag in the Qt configs

This comment has been minimized.

Sign in to view

Copy link
@rryan

rryan Nov 18, 2015

Member

Could you clarify what "may work or not" means here? Is this for Qt4 vs Qt5 compatibility? (AIUI Qt4 is always built with /Zc:wchar_t- on Windows while Qt5 is not).

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Nov 18, 2015

Author Member

If Mixxx and the called code in a library share the same /Zc:wchar_t flags, you can use wchar_t.
If not, it depends on the function interface.

This comment has been minimized.

Sign in to view

Copy link
@rryan

rryan Nov 18, 2015

Member

Could you clarify why you made the change though? Was it for qt4 vs. qt5? Were you dealing with a different library that we build with /Zc:wchar_t?

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Nov 18, 2015

Author Member

We build Taglib original without a flag, which uses the build in type for wchar_t. Since Mixxx was like Qt4 build with /Zc:wchar_t- flag we could not use Taglibs wchar_t functions.
We could fix the issue by compiling Taglib with /Zc:wchar_t- or removing /Zc:wchar_t- from Mixxx.
We decided for the later, since we are forced to do this anyway when switching to QT5.
If we now just not use Qts wchat_t functions like here, we are in the lucky situation that we do not need to switch the /Zc:wchar_t- flag when switching Qt versions.

This comment has been minimized.

Sign in to view

Copy link
@rryan

rryan via email Nov 18, 2015

Member
if (sizeof(wchar_t) == sizeof(QChar)) {
return QString::fromUtf16((const ushort *)pStr, (int)max_length);
} else {
return QString::fromUcs4((uint *)pStr, (int)max_length);

This comment has been minimized.

Sign in to view

Copy link
@rryan

rryan Nov 18, 2015

Member

When is this path possible on Windows? mingw?

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Nov 18, 2015

Author Member

Windows will use the first branch, unless Qt changes the size of QChar or windows changes the size of wchar_t
This condition is used inside the QT source as well.
Normally Linux will not use the first branch since sizeof(wchar_t) is four, but any distro is free to change this.

This comment has been minimized.

Sign in to view

Copy link
@rryan

rryan Nov 18, 2015

Member

Gotcha -- the condition is used in Qt but not with a WINDOWS ifdef. If we're going to inline fromWCharArray we may as well keep it simple and inline it on all platforms. I think it's confusing to have an else branch that can't happen.

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Nov 18, 2015

Author Member

I thought about it as well and I was unsure. I pick the ifdef solution because we can remove it after switching to QT5. I will remove the ifdef for clarity since there is no benefit to revert it in the Qt5 case.

This comment has been minimized.

Sign in to view

Copy link
@daschuer

daschuer Nov 18, 2015

Author Member

see: 67d56f5
}
// pStr is untrusted since it might be non-null terminated.
wchar_t* tmp = new wchar_t[max_length+1];
// wcsnlen is not available on all platforms, so just make a temporary
// buffer
wcsncpy(tmp, pStr, max_length);
tmp[max_length] = 0;
QString result = QString::fromWCharArray(tmp);
delete [] tmp;
return result;
#else
return QString::fromWCharArray(pStr, (int)max_length);
#endif
}

HidController::HidController(const hid_device_info deviceInfo)
Expand Down Expand Up @@ -335,10 +334,10 @@ void HidController::send(QByteArray data, unsigned int reportID) {
if (debugging()) {
qWarning() << "Unable to send data to" << getName()
<< "serial #" << hid_serial << ":"
<< QString::fromWCharArray(hid_error(m_pHidDevice));
<< safeDecodeWideString(hid_error(m_pHidDevice), 512);
} else {
qWarning() << "Unable to send data to" << getName() << ":"
<< QString::fromWCharArray(hid_error(m_pHidDevice));
<< safeDecodeWideString(hid_error(m_pHidDevice), 512);
}
} else if (debugging()) {
qDebug() << result << "bytes sent to" << getName()
Expand Down

0 comments on commit 344ae07

Please sign in to comment.