On terça-feira, 5 de fevereiro de 2013 11.33.29, Paul Olav Tvete wrote: > M config.tests/qpa/linuxfb/linuxfb.cpp > M config.tests/unix/arch.test
+2 up to here. > M config.tests/unix/compile.test This one has superfluous changes. It sets a QMAKE variable that is not used anywhere. > M config.tests/unix/evdev/evdev.cpp > M config.tests/unix/getaddrinfo/getaddrinfotest.cpp +2 And +ugh! on the getaddrinfotest.cpp. Recheck if it's necessary. Drop if it's not. > A config.tests/unix/ipc_android/ipc.cpp > A config.tests/unix/ipc_android/ipc_android.pro Questionable copyright on the .cpp file. Looks like it's checking for linux/ashmem.h and some macros. > M config.tests/unix/makeabs +1. It's about cross-compilation from windows to unix. > M configure +1 on the msys part, but I can't approve. -1 on the detection of android. It detects android via one single mkspec name, which is hardcoded. This should be done by a getQMakeConf call and extracting a variable set there, if necessary. Or by a compile test, like the ashmem above. -1 on the unnecessary whitespace changes. Except that I can't find where they were introduced... might be an artifact of my diff. -1 on this: +if [ "$PLATFORM" = "win32-g++" ] || [ "$XPLATFORM" = "win32-g++" ] && [ "$CFG_MS_BITFIELDS" = "yes" ]; then + QMAKE_CONFIG="$QMAKE_CONFIG ms_bitfields" +fi The above enables ms_bitfields unconditionally for MinGW, regardless of cross- compilation or anything else. You're missing parentheses. -2 on the -openssl-source option. It's not used anywhere. > A lib/rules.xml I'd rather you found a better place for this file. > A mkspecs/android-g++/qmake.conf -1 on CONFIG += ashmem. It looks wrong. -2 on QT = android. You can't have "android" there, period. That belongs to a library called QtAndroid, if and when that exists, and it MUST NOT be default. If you need that for initialisation, figure out like qtmain / winmain does. Just don't touch the QT variable. The file has some weird indentation. -1 on setting -g on the release flags. This produces an unlinkable QtWebKit. > A mkspecs/android-g++/qplatformdefs.h > D mkspecs/common/android/qplatformdefs.h > D mkspecs/common/linux-android.conf Please keep the common files. > A mkspecs/features/android.prf Copyright header is out of style. Please use one in our format. > A mkspecs/features/java.prf Ossi to judge if the mkpath should be there. javac.depends looks like a horrible hack. In fact, while this is a nice feature, the whole file looks like a hack. qmake should be modified to learn how to compile Java instead. > M mkspecs/features/moc.prf -1. The change looks wrong. It needs justification. > A mkspecs/features/qpa/android.prf $$[QT_INSTALL_PREFIX]/src makes no sense. Please choose another path. Also, doesn't qpa/android.prf conflict with android.prf? > M mkspecs/features/qpa/genericunixfontdatabase.prf -1 on coding style. > M mkspecs/features/qt_functions.prf -2 for reasons explained above. Do not modify the QT variable. > M mkspecs/features/qt_parts.prf Changes look incorrect at first glance. Ossi to judge. > M mkspecs/features/resources.prf As the comment says, proper investigation should be done. If it weren't for the comment, I'd have given +2. > M mkspecs/features/static.prf There's no linux-mingw-*. I'd like to see this made more generic: add a variable to the qmake.conf instead. I can then add -static-intel for icc, for example. > D mkspecs/unsupported/linux-android-armeabi-g++/qmake.conf > D mkspecs/unsupported/linux-android-armeabi-g++/qplatformdefs.h > D mkspecs/unsupported/linux-android-armeabi-v7a-g++/qmake.conf > D mkspecs/unsupported/linux-android-armeabi-v7a-g++/qplatformdefs.h > D mkspecs/unsupported/linux-android-x86-g++/qmake.conf > D mkspecs/unsupported/linux-android-x86-g++/qplatformdefs.h Ok, it's supported now. +2 > M qmake/generators/makefile.cpp > M qmake/generators/unix/unixmake2.cpp > M qmake/generators/win32/mingw_make.cpp > M qmake/generators/win32/msvc_nmake.cpp Part of the make-qmake-understand-Java, but incomplete. See above. > M qtbase.pro Unnecessary change. > A src/android/... I suggest the dir be renamed to src/androidmain, to match winmain and the old (and thankfully removed) s60main. > M src/corelib/arch/arch.pri > A src/corelib/arch/qatomic_android.h -1. Looks superfluous. It's doing exactly what qbasicatomic.h is already doing. Please justify. > M src/corelib/codecs/qiconvcodec.cpp +2 > M src/corelib/codecs/qiconvcodec_p.h +1 but Q_OS_WIN && Q_CC_GNU. > M src/corelib/codecs/qtextcodec.cpp > M src/corelib/codecs/qtextcodec_p.h This has been discussed before. Please leave the macro called Q_OS_LINUX_ANDROID. >From this point on, any +2 does is modulo the macro name being changed back. > M src/corelib/global/qglobal.cpp > M src/corelib/global/qlogging.cpp +2 > M src/corelib/global/qsystemdetection.h -1 Macro name. See above. > M src/corelib/io/qabstractfileengine_p.h > M src/corelib/io/qfilesystemengine_unix.cpp +2 > M src/corelib/io/qfsfileengine_unix.cpp -1. You need to add a fallback. > M src/corelib/io/qtemporarydir.cpp -1. It looks like Android has mkdtemp, it's just not defined in the headers. > M src/corelib/kernel/kernel.pri 0 > M src/corelib/kernel/qeventdispatcher_unix.cpp > M src/corelib/kernel/qeventdispatcher_unix_p.h This one needs a very good explanation. > M src/corelib/kernel/qsharedmemory.cpp +2 > D src/corelib/kernel/qsharedmemory_android.cpp -2. Keep the name, especially since there's qsystemsemaphore_android.cpp. > A src/corelib/kernel/qsharedmemory_ashmem.cpp -1: * need to use qt_safe_{open,close} * why the strlcopy? Can't it pass the name parameter? * "static" missing in all functions * coding style * there's a QString creation between the failing system call and checking of errno. That is a bad idea, since malloc() can modify errno. Refactor setErrorString. -> additionally, merge it with the _unix implementation. * the pin / unpin functions seem to be wrongly used > M src/corelib/kernel/qsharedmemory_p.h -1: * coding style (spaces in preprocessor macros) * the nested #ifdef look bad. You can do better. > M src/corelib/kernel/qsystemsemaphore.cpp +2 > M src/corelib/kernel/qsystemsemaphore_android.cpp -1: * code duplication (setErrorString again) * coding style > M src/corelib/kernel/qsystemsemaphore_p.h -1: coding style > M src/corelib/thread/qbasicatomic.h -1. See above. > A src/corelib/tools/android/cpu-features.c > A src/corelib/tools/android/cpu-features.h > M src/corelib/tools/qsimd.cpp > M src/corelib/tools/tools.pri Please explain why this is necessary. The checking in qsimd.cpp should be enough already. Move new files to 3rdparty if still valid. > M src/gui/image/qimage_neon.cpp -2. Looks wrong. qsimd_p.h already does that include and this won't compile on Android-x86. > M src/gui/kernel/qplatforminputcontext.cpp > M src/gui/kernel/qplatforminputcontext.h -2. Superfluous and adds a warning. > M src/network/access/qnetworkaccessfilebackend.cpp > M src/network/access/qnetworkaccessmanager.cpp > M src/network/access/qnetworkreplyfileimpl.cpp #ifdef missing. > M src/network/kernel/qhostinfo_unix.cpp -2. Fix the configure test instead. > M src/network/kernel/qnetworkproxy_win.cpp -2. The Android assets will never be run on Windows. > M src/network/ssl/qsslcertificate.cpp +2 > M src/network/ssl/qsslsocket_openssl.cpp -1: coding style > M src/network/ssl/ssl.pri -1: this unconditionally builds OpenSSL support in, even if the sources aren't present. The configure script changes required for this aren't correct. > A src/plugins/platforms/android/... Everything with AOSP or Khronos copyright: move to 3rdparty. asset file engine: I don't like it. I need to think some more about it. > M src/plugins/platforms/cocoa/qcocoamenu.h > M src/plugins/platforms/cocoa/qcocoamenuitem.h -1: coding style > M src/plugins/platforms/platforms.pro -1: superfluous commented out line > M src/src.pro See above about the naming. > M src/widgets/graphicsview/qgraphicsitem.cpp > M src/widgets/itemviews/qitemdelegate.cpp > M src/widgets/kernel/qwidget.cpp -1 on OS-based policy decisions. This should be in QPA. > A src/widgets/styles/json/json.cpp > A src/widgets/styles/json/json.g > A src/widgets/styles/json/json.h > A src/widgets/styles/json/json.pri > A src/widgets/styles/json/jsonparser.cpp -1. Do not add. You need to show that this is at least 5x faster than the one in QtCore before this begins to be acceptable. I'm not the QtWidgets maintainer. If I were, I'd be giving a -2. If somehow this is kept, it needs to be moved to 3rdparty. > M src/widgets/styles/qcommonstyle.cpp -1: OS-based policy decisions. > M src/widgets/styles/qstylefactory.cpp > M src/widgets/styles/styles.pri +1, except for json. > M src/widgets/widgets/qabstractbutton.cpp > M src/widgets/widgets/qcombobox.cpp -1: OS-based policy decisions. > M src/widgets/widgets/qmenu.cpp +2. > M src/widgets/widgets/qtextbrowser.cpp #ifdef on assets -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development