Port endl to "\n". endl in qt5.15 is namespaced. We don't need to flush as when QFile is deleted it flush data
ClosedPublic

Authored by mlaurent on Dec 31 2019, 6:00 AM.

Diff Detail

Repository
R238 KDocTools
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mlaurent created this revision.Dec 31 2019, 6:00 AM
Restricted Application added projects: Frameworks, Documentation. · View Herald TranscriptDec 31 2019, 6:00 AM
Restricted Application added subscribers: kde-doc-english, kde-frameworks-devel. · View Herald Transcript
mlaurent requested review of this revision.Dec 31 2019, 6:00 AM

Will be better if you use QLatin1Char('\n'), "\n" will call strlen on which is unneeded. Also QStringLiteral is missing on some strings.

mlaurent updated this revision to Diff 72444.Dec 31 2019, 7:33 AM

use QLatin1Char('\n')

"Also QStringLiteral is missing on some strings." it can be fix in other patch.

aacid added a subscriber: aacid.Dec 31 2019, 11:21 AM

Isn't it better to just use Qt::endl ?

I think it's much clearer to understand Qt::endl than QLatin1Char('\n')

But if we prefer to change to use \n it should be merged into the existing strings, doesn't make much sense to do
outStream << "</l:i18n>" << QLatin1Char('\n');
instead of
outStream << "</l:i18n>\n';

Isn't it better to just use Qt::endl ?

No, it's not. It's namespaced in Qt 5.15 means it should be ifdef guarded which makes things to suck.

But it is already namespaced in older Qt too, so no ifdef needed

"No, it's not. It's namespaced in Qt 5.15 means it should be ifdef guarded which makes things to suck." yep it's for that I switched to "\n"

I don't want to add #ifdef if it's not necessary.

"/compile/kde5/framework/frameworks/kdoctools/src/docbookl10nhelper.cpp: Dans la fonction « int writeLangFile(const QString&, const QString&, const LangListType&) »:
/compile/kde5/framework/frameworks/kdoctools/src/docbookl10nhelper.cpp:62:57: erreur: « endl » n'est pas un membre de « Qt »

62 |               .arg(dtdPath) << QLatin1Char('\n') << Qt::endl;

"
no it's not :) qt5.13 :)

Ah, it works fine in 5.14.

I was going to say we must report to Qt immediately that they broke source compatibility and that was unacceptable but then i realized that the old non namespaced version works just fine, it's just deprecated.

I'm going to go back to party preparations :)

Isn't it better to just use Qt::endl ?

I think it's much clearer to understand Qt::endl than QLatin1Char('\n')

But if we prefer to change to use \n it should be merged into the existing strings, doesn't make much sense to do
outStream << "</l:i18n>" << QLatin1Char('\n');
instead of
outStream << "</l:i18n>\n';

I agree with Albert here; I think it's a bit more painful but it's probably going to be cleaner.

mlaurent updated this revision to Diff 72514.Jan 1 2020, 9:43 AM

merge \n

aacid accepted this revision.Jan 1 2020, 12:04 PM
This revision is now accepted and ready to land.Jan 1 2020, 12:04 PM
This revision was automatically updated to reflect the committed changes.