⚙ D9420 Compile commenting remove_defintion(QT_NO_CAST_FROM

velurimithun created this revision.Dec 20 2017, 8:16 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptDec 20 2017, 8:16 AM

Restricted Application added a subscriber: Frameworks. · View Herald Transcript

velurimithun requested review of this revision.Dec 20 2017, 8:16 AM

Errors in Kaboutdataappplicationdatatest file

Could you paste you compile error here please.

autotests/CMakeLists.txt
0–1	Better to remove it.

mlaurent requested changes to this revision.Dec 20 2017, 9:00 AM

This revision now requires changes to proceed.Dec 20 2017, 9:00 AM

/home/veluri/ruqola_dep/kcoreaddons/autotests/kaboutdataapplicationdatatest.cpp:72: error: request for member ‘setOrganizationDomain’ in ‘aboutData2’, which is of non-class type ‘KAboutData(QLatin1String, QLatin1String, QLatin1String)’

aboutData2.setOrganizationDomain(OrganizationDomain2);
           ^

/home/veluri/ruqola_dep/kcoreaddons/autotests/kaboutdataapplicationdatatest.cpp:73: error: request for member ‘setDesktopFileName’ in ‘aboutData2’, which is of non-class type ‘KAboutData(QLatin1String, QLatin1String, QLatin1String)’

aboutData2.setDesktopFileName(QLatin1String(DesktopFileName2));
           ^

/home/veluri/ruqola_dep/kcoreaddons/autotests/kaboutdataapplicationdatatest.cpp:75: error: no matching function for call to ‘KAboutData::setApplicationData(KAboutData (&)(QLatin1String, QLatin1String, QLatin1String))’

KAboutData::setApplicationData(aboutData2);
                                         ^

KAboutData aboutData2(QString::fromLatin1(AppName2), QString::fromLatin1(ProgramName2), QString::fromLatin1(Version2));

use QString::fromLatin1 is will work

Arguiment from method use QString and not QLatin1String()
QString::fromLatin1 returns a QString

velurimithun retitled this revision from Compile commenting remove_defintion(QT_NO_CAST_FROM_ASCII defination) to Compile commenting remove_defintion(QT_NO_CAST_FROM_ASCII ).Dec 20 2017, 3:17 PM

use QLating1String

mlaurent requested changes to this revision.Dec 20 2017, 5:54 PM

mlaurent added inline comments.

autotests/kaboutdatatest.cpp
61	no we need to keep const char ...[] and use QString::fromLatin1(...) in method as it's already done previously
autotests/kautosavefiletest.cpp
83	QStringLiteral(...)
autotests/kformattest.cpp
36–44	QStringLiteral here and others.
308	QStringLiteral here too
autotests/kpluginfactorytest.cpp
34	QStringLiteral(...)
autotests/kpluginmetadatatest.cpp
49	Same
207–208	Same
autotests/kshelltest.cpp
58–61	QStringLiteral...
84	you can change QString::fromLatin1(...) by QStringLiteral in all code that you modified

This revision now requires changes to proceed.Dec 20 2017, 5:54 PM

Changes are made using QStringLiteral

Update the code using QStringLiteral

Missing all patch... please see "https://community.kde.org/Infrastructure/Phabricator"

"Step 2: Updating your diff.

After you upload the code the reviewer will take a look and give you some comments. If you get a thumbs up, you can skip this step. But often you will get a review like "looks good, we can take it if you fix problems x, y, z." After you make your changes, you can add an extra commit to the git branch.

$ git add -u
$ git commit
$ arc diff
"

velurimithun updated this revision to Diff 24199.Dec 21 2017, 5:58 AM

I'm getting errors in processtest!!

/home/veluri/Qt/5.9.3/gcc_64/include/QtCore/qstringbuilder.h:357: error: no matching function for call to ‘QConcatenable<char [16]>::appendTo(const char [16], QChar*&)’

QConcatenable<A>::appendTo(p.a, out);
                          ^

same kind of errors at 69, 71, 73

mlaurent added inline comments.Dec 21 2017, 6:38 AM

autotests/desktoptojsontest.cpp
84	not necessary to change indentation in this patch.
167	don't change indent please
autotests/kaboutdatatest.cpp
184	QStringLiteral doen't work here ?
autotests/kprocesstest.cpp
93	QStringLiteral

velurimithun marked an inline comment as done.Dec 21 2017, 6:44 AM

velurimithun added inline comments.

autotests/kaboutdatatest.cpp
184	Yeah working, I'll change it
autotests/kprocesstest.cpp
93	These strings are going as arguments for QStringList() method that why I used QString::FromLatin1()

mlaurent added inline comments.Dec 21 2017, 6:52 AM

autotests/kprocesstest.cpp
93	? QStringLiteral works fine with QStringList

velurimithun updated this revision to Diff 24238.Dec 21 2017, 3:17 PM

velurimithun marked 2 inline comments as done.

You still missing all patch...
Please use arc diff so we are sure that all patch is updated.

no news ?

patch created for all the changes from starting

This diff contains total changes in all files...

kcoreaddons compilation done!!

velurimithun added inline comments.Dec 29 2017, 12:04 PM

autotests/kprocesstest.cpp
93	Yeah working, I changed it But, Unable to eliminate this compile error
autotests/kstringhandlertest.cpp
45	Here char i. e., (' ') is converted into string i.e., (" ") and then QLatin1String is applied. Since, we can't convert char into QLatin1String
76	Here too zwsp was QChar, it is converted into QString. Same for below lines also!!

mlaurent added inline comments.Dec 30 2017, 10:24 AM

autotests/kprocesstest.cpp
47	nocrashhandler why do you changed as QString::fromLatin1 ? it's QStringList() so it can work no ?
autotests/kshelltest.cpp
167	QLAtin1String for japan char ?
autotests/kstringhandlertest.cpp
45	QLatin1Char(' ') ?
49–51	QStringLiteral no ?

Make necessary changes according to comments in D9420

Use QLatin1Char for char

velurimithun marked 4 inline comments as done.Dec 30 2017, 4:23 PM

Hi!!,

Code updated. Please let me know any changes are there yet to be done

Regards.
VM

The rest looks good.

autotests/kshelltest.cpp
139–140	Does this test still pass? You can't use QStringLiteral when there are escape characters like \x21 in the literal. See https://commits.kde.org/clazy/49825f06fda23989961cfa41cd1576b58e2bcc5d for details. Please make sure to run `ctest` before and after.

indeed code will be broken if we don't use QString::fromUtf8.
Could you fix it please ?

Use QString::fromUtf8(..) to decode an escape sequence like \x21

Fixed :)

StringEncodings doc helped me to understand encoding and decoding.

Still if there any modifications kindly let me know.

Thank You.

When I try to compile your patch I have this compile error:
Do you are sure that you rebuild all ?

/compile/kde5/framework/frameworks/kcoreaddons/autotests/kaboutdatatest.cpp: In member function ‘void KAboutDataTest::testLicenseSPDXID()’:
/compile/kde5/framework/frameworks/kcoreaddons/autotests/kaboutdatatest.cpp:330:91: error: ‘QString::QString(const char*)’ is private within this context

QLatin1String(ShortDescription), KAboutLicense::GPL_V2);
                                                      ^

In file included from /opt/qt5.10/include/QtCore/QString:1:0,

from /compile/kde5/framework/frameworks/kcoreaddons/src/lib/kaboutdata.h:30,
from /compile/kde5/framework/frameworks/kcoreaddons/autotests/kaboutdatatest.cpp:21:

/opt/qt5.10/include/QtCore/qstring.h:818:5: note: declared private here

QString(const char *ch);
^~~~~~~

/compile/kde5/framework/frameworks/kcoreaddons/autotests/kaboutdatatest.cpp:330:91: error: ‘QString::QString(const char*)’ is private within this context

QLatin1String(ShortDescription), KAboutLicense::GPL_V2);
                                                      ^

In file included from /opt/qt5.10/include/QtCore/QString:1:0,

from /compile/kde5/framework/frameworks/kcoreaddons/src/lib/kaboutdata.h:30,
from /compile/kde5/framework/frameworks/kcoreaddons/autotests/kaboutdatatest.cpp:21:

/opt/qt5.10/include/QtCore/qstring.h:818:5: note: declared private here

This revision now requires changes to proceed.Jan 11 2018, 6:33 AM

Yeah, I have rebuilt it !! NO ERRORS compiling well. I'm not getting any errors like that :/

I could see few warnings but not in kaboutdatatest.cpp

I'm compiling it in Qt 5.9.3 version.

Ok I understand why you don't have this problem you need to rebase patch with last kcoreaddons version.
So code was added.
Please rebase, make sure that it builds and upload patch.
Regards.

Ping ?

Ping bis ?:)

use QLating1String
compile commenting remove_definition(DQT_NO_CAST_FROM_ASCII
Make necessary changes according to comments in D9420
Use QString::fromUtf8(..) to decode an escape sequence like \x21
Merge with origin/master
Rewrite strings using QLatin1String() and QString::fromLatin1()

Sry for the late patch :))

I was busy with setting up Ruqola...

I have one doubt that why this(kdirwatch_unittest.cpp ) file is in autotests?? It should be in unit tests right?

Thank You for your patience.

"autotests" and unittests are the same thing. The files are in the right place.

You still forgot to launch autotest...
see:
"laurent@linux-5nvn:/compile/kde5/framework/frameworks/kcoreaddons/build> ./bin/kaboutdatatest

Start testing of KAboutDataTest *****

Config: Using QtTest library 5.10.1, Qt 5.10.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.2.1 20171020 [gcc-7-branch revision 253932])
PASS : KAboutDataTest::initTestCase()
PASS : KAboutDataTest::testLongFormConstructorWithDefaults()
PASS : KAboutDataTest::testLongFormConstructor()
PASS : KAboutDataTest::testShortFormConstructor()
FAIL! : KAboutDataTest::testSetAddLicense() Compared values are not the same

Actual   (aboutData.licenses().at(2).text()): "free to write, reading forbidden"
Expected (QLatin1String(LicenseFileText))   : "free to write, reading forbidden, in the file"
Loc: [/compile/kde5/framework/frameworks/kcoreaddons/autotests/kaboutdatatest.cpp(260)]

PASS : KAboutDataTest::testSetProgramIconName()
PASS : KAboutDataTest::testSetDesktopFileName()
PASS : KAboutDataTest::testCopying()
PASS : KAboutDataTest::testKAboutDataOrganizationDomain()
PASS : KAboutDataTest::testLicenseSPDXID()
PASS : KAboutDataTest::testLicenseOrLater()
PASS : KAboutDataTest::cleanupTestCase()
Totals: 11 passed, 1 failed, 0 skipped, 0 blacklisted, 3ms
"

> I found the porting error in line 260. Please compare you change in 260.

You must launch autotest to see if you created a bug or not.

You need to compare before and after your patch and see if you have some failed.

This revision now requires changes to proceed.Jan 21 2018, 11:27 AM

Compare license file correctly

Note:

In kdirwatch_qfswatch_unittest KDirWatch_UnitTest::benchNotifyWatcher() fails before and after changes in kdirwatch_unittest.cpp file

Thanks

This revision is now accepted and ready to land.Jan 22 2018, 12:21 PM

Closed by commit R244:53f3dce3f012: Compile commenting remove_defintion(QT_NO_CAST_FROM_ASCII ) (authored by velurimithun). · Explain WhyJan 22 2018, 12:41 PM

This revision was automatically updated to reflect the committed changes.

This fails to build on Windows, please see https://build.kde.org/job/Frameworks%20kcoreaddons%20kf5-qt5%20WindowsMSVCQt5.9/49/consoleText
Can you please look into and commit a fix for this as soon as possible?

I made a fix. I wait a rebuild on windows to see if it's ok

			Path	Packages
M			autotests/CMakeLists.txt (2 lines)
M			autotests/desktoptojsontest.cpp (344 lines)
M			autotests/kaboutdataapplicationdatatest.cpp (2 lines)
M			autotests/kaboutdatatest.cpp (110 lines)
M			autotests/kautosavefiletest.cpp (2 lines)
M			autotests/kdelibs4migrationtest.cpp (5 lines)
M			autotests/kdirwatch_unittest.cpp (2 lines)
M			autotests/kformattest.cpp (338 lines)
M			autotests/kpluginfactorytest.cpp (8 lines)
M			autotests/kpluginloadertest.cpp (162 lines)
M			autotests/kpluginmetadatatest.cpp (185 lines)
M			autotests/kprocesstest.cpp (23 lines)
M			autotests/krandomtest.cpp (4 lines)
M			autotests/kshelltest.cpp (74 lines)
M			autotests/kstringhandlertest.cpp (63 lines)
M			autotests/ktexttohtmltest.cpp (82 lines)
M			autotests/kusertest.cpp (4 lines)
M			autotests/multiplugin.cpp (6 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	24143	fbc5881		Dec 20 2017, 8:16 AM	★	★
Diff 2	24175	fbc5881	- use QLating1String	Dec 20 2017, 5:08 PM	★	★
Diff 3	24197	4e6b2e4	Changes are made using QStringLiteral	Dec 21 2017, 5:45 AM	★	★
Diff 4	24198	4e6b2e4	Update the code using QStringLiteral	Dec 21 2017, 5:49 AM	★	★
Diff 5	24199			Dec 21 2017, 5:58 AM	★	★
Diff 6	24238			Dec 21 2017, 3:16 PM	★	★
Diff 7	24452	fbc5881	patch created for all the changes from starting	Dec 29 2017, 11:54 AM	★	★
Diff 8	24493	fbc5881	- Make necessary changes according to comments in D9420	Dec 30 2017, 4:22 PM	★	★
Diff 9	25076	fbc5881	- Use QString::fromUtf8(..) to decode an escape sequence like \x21	Jan 10 2018, 11:42 AM	★	★
Diff 10	25677	16e8cf1	- use QLating1String	Jan 20 2018, 10:33 AM	★	★
Diff 11	25712	16e8cf1	- Compare license file correctly	Jan 21 2018, 12:09 PM	★	★
Diff 12	25751	78e9ffa	R244:53f3dce3f012c69c16f88c5a5eec8f4c5fa96274	Jan 22 2018, 12:41 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
bd1abe80ed64	0d6b63a9efcf	276240a146ac	Veluri Mithun	Compare license file correctly	Jan 21 2018, 12:07 PM
276240a146ac	4b94d70b8afd	1eced6dac2b5	Veluri Mithun	Rewrite strings using QLatin1String() and QString::fromLatin1()	Jan 20 2018, 10:29 AM
1eced6dac2b5	2b596ce60409	9cf0c955041a	Veluri Mithun	Merge with origin/master	Jan 19 2018, 3:30 PM
9cf0c955041a	db0c83305507	22a07f1eb6e1	Veluri Mithun	Use QString::fromUtf8(..) to decode an escape sequence like \x21	Jan 10 2018, 11:38 AM
22a07f1eb6e1	5e3ecb34e011	613df953fd21	Veluri Mithun	Make necessary changes according to comments in D9420	Dec 30 2017, 4:16 PM
613df953fd21	bde4766a1aab	226c373c4173	Veluri Mithun	compile commenting remove_definition(DQT_NO_CAST_FROM_ASCII	Dec 29 2017, 11:41 AM
226c373c4173	25ed4fd2525f	1458c9dc8bc7	Veluri Mithun	use QLating1String	Dec 20 2017, 5:06 PM
1458c9dc8bc7	51611e7280ff	16e8cf1e6607	Veluri Mithun	Compile commenting remove_defintion(QT_NO_CAST_FROM_ASCII defination) (Show More…)	Dec 20 2017, 8:10 AM

Compile commenting remove_defintion(QT_NO_CAST_FROM_ASCII )
ClosedPublic
Actions

Details

Diff Detail

> I found the porting error in line 260. Please compare you change in 260.

Revision Contents
Changeset List

Diff 25712

autotests/CMakeLists.txt

autotests/desktoptojsontest.cpp

autotests/kaboutdataapplicationdatatest.cpp

autotests/kaboutdatatest.cpp

autotests/kautosavefiletest.cpp

autotests/kdelibs4migrationtest.cpp

autotests/kdirwatch_unittest.cpp

autotests/kformattest.cpp

autotests/kpluginfactorytest.cpp

autotests/kpluginloadertest.cpp

autotests/kpluginmetadatatest.cpp

autotests/kprocesstest.cpp

autotests/krandomtest.cpp

autotests/kshelltest.cpp

autotests/kstringhandlertest.cpp

autotests/ktexttohtmltest.cpp

autotests/kusertest.cpp

autotests/multiplugin.cpp

Compile commenting remove_defintion(QT_NO_CAST_FROM_ASCII )ClosedPublicActions

Details

Diff Detail

> I found the porting error in line 260. Please compare you change in 260.

Revision ContentsChangeset List

Diff 25712

autotests/CMakeLists.txt

autotests/desktoptojsontest.cpp

autotests/kaboutdataapplicationdatatest.cpp

autotests/kaboutdatatest.cpp

autotests/kautosavefiletest.cpp

autotests/kdelibs4migrationtest.cpp

autotests/kdirwatch_unittest.cpp

autotests/kformattest.cpp

autotests/kpluginfactorytest.cpp

autotests/kpluginloadertest.cpp

autotests/kpluginmetadatatest.cpp

autotests/kprocesstest.cpp

autotests/krandomtest.cpp

autotests/kshelltest.cpp

autotests/kstringhandlertest.cpp

autotests/ktexttohtmltest.cpp

autotests/kusertest.cpp

autotests/multiplugin.cpp

Compile commenting remove_defintion(QT_NO_CAST_FROM_ASCII )
ClosedPublic
Actions

Revision Contents
Changeset List