@mlaurent Bonne année :) Hm, are we sure that endl is namespaced with Qt:: in 5.15 already? Isn't that rather Qt6 only?

For Qt5 endl is in the namespace QTextStreamFunctions rather, which though is also inlined by using namespace QTextStreamFunctions, so that should still compile, no?
IMHO this should be rather only ported/touched during Qt6 then. Well, unless doing the flush only at the end is an improvement of course.

See https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/serialization/qtextstream.h:

#if defined(Q_QDOC) || QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
namespace Qt {
#else
// This namespace only exists for 'using namespace' declarations.
namespace QTextStreamFunctions {
#endif
// [...]
Q_CORE_EXPORT QTextStream &endl(QTextStream &s);
// [...]

} // namespace QTextStreamFunctions

#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0) && !defined(Q_QDOC)
namespace Qt {
using namespace QTextStreamFunctions;
}

QT_WARNING_PUSH
QT_WARNING_DISABLE_CLANG("-Wheader-hygiene")
// We use 'using namespace' as that doesn't cause
// conflicting definitions compiler errors.
using namespace QTextStreamFunctions;
QT_WARNING_POP
#endif // QT_VERSION < QT_VERSION_CHECK(6, 0, 0) && !defined(Q_QDOC)

@kossebau bonne année aussi :) Yep for sure it's namespaced in qt5.15 (I build it monday from qt5.15 branch)

Ah, I looked at the "dev" branch only and missed that it is possibly already pre-Qt6, no longer base for any future Qt5 branches, right?
Indeed things look differently in the 5.15 branch, where the old variant is tagged deprecated, I now see.

Have to tell that I am bit unhappy about what Qt devs are forcing us here into then, only because we also want to support versions < 5.15:
assuming that all those end usages are correct and preferred, this code here once ported to Qt6 should simply use Qt::endl again, right?
The only reason we do these changes here is to avoid deprecation warnings.
And I agree that doing any #if QT_IS_VERSION_5_15 Qt::endl #ELSE endl #ENDIF sucks here, as there are too many instances and embedded too deep in other expressions.

But now simply dropping the use of endl instead, so departing from a well-known C++ code pattern, into doing lots of explicit calls (<< QLatin1Char('\n') + flush()) makes the code worse IMHO.

No other idea what to do here yet. Perhaps we could instead do an alias definition in each affected file instead, so having only one place with #if QT_IS_VERSION_5_15 Qt::endl #ELSE endl #ENDIF. Like (untested sketched code):

namespace KF {
#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
QTextStream &endl(QTextStream &s) { return Qt::endl(s); }
#ELSE
QTextStream &endl(QTextStream &s) { return endl(s); }
#ENDIF
}

stream << "something for output" << KF::endl;

So the current logic with the known patterns could stay, and once KF6 is created (so backward compat code can be dropped), things could be simply changed to be just Qt::endl.

What do you think?

(I start to think that there are cases where having sub-version-level control about deprecations warnings would be a reasonable feature...)

I don't think a code generator needs to flush() many times along the way anyway. \n actually sounds more efficient.

@mlaurent Hm, is there a chance ABI got broken by that respective code in Qt 5.15 when it comes to the non-Qt-namespaced variants of the methods? Unless I misread, those deprecated ones are now in the QTextStreamFunctions namespace. And which might be source compatible, due to the using namespace QTextStreamFunctions; in the header, but does that help with the namespace being reflected in the exported symbols?

Can you do some nm -gDC /usr/lib64/libQt5Core.so.5.y.z | grep endl (with y.z matching the actual numbers of the Qt 5.15 libs you have) and tell what exported symbols there are. by the example of endl?

nm -gDC libQt5Core.so.5.15.0 | grep endl
0000000000336680 T endl(QTextStream&)
0000000000336500 T QTextStreamFunctions::endl(QTextStream&)
00000000003361e0 T Qt::endl(QTextStream&)

Thanks. Okay, so the old symbol is still there, and with that info I looked some more, and indeed the ::endl symbol is not declared in the header qtextstream.h, but only defined (and exported) in the source file qtextstream.cpp. Having 3 variants now does not make sense to me on first look, but perhaps a result of some incomplete approach in Qt 5.14... oh well...

For Okteta I now went for this approach, which I find more elegant:

using TextStreamFunction = QTextStream& (*)(QTextStream&);
#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
constexpr TextStreamFunction endl = Qt::endl;
#else
constexpr TextStreamFunction endl = ::endl;
#endif

No extra namespace here, as all code is already in some Okteta namespace, so the lines with the endl do not even have to be touched :)

Whether endl is a proper call in such places is left for another investigation on its own.

In D26320#585916, @dfaure wrote:

I don't think a code generator needs to flush() many times along the way anyway. \n actually sounds more efficient.

Do Qt experts have some numbers on the sizes before which flush() makes sense in general? I guess it varies on the actual QIODevice, and then with QFile perhaps on the actual filesystem/kernel/whatever?

Also surprised to see the implementation do a ¸writeBuffer.replace(QLatin1Char('\n'), QLatin1String("\r\n"));¸for windows where I expected endl itself to care for it...

Doesn't flushing happen automatically when internal buffers are full enough? i.e. I don't think the app should have to think about that, except when it *wants* partial results to be visible (which isn't the case in a code generator).

About replace('\n', '\r\n') vs endl doing that, well, this is what makes it possible to use "\n" in code and still get \r\n on Windows :)

So what I need to change in this patch ? (if I need to change a code :) )

No change needed ? I will commit in two days if no complains.

In D26320#586935, @mlaurent wrote:

So what I need to change in this patch ? (if I need to change a code :) )

When it comes to me, so far I have been more curious what the best principled approach is in general to react to the deprecation challenge we have here.
And what the perfect streaming code would look like in general, now that we are touching this code.

As said initial, I am used to the pattern to "<< endl" everywhere and assume some others are as well. Having now code doing all kind of custom linebreaks and flushing only when thinking it is needed or relying on auto-flush in other cases by destructors makes things seem more complicated to me, because less patterns and more chances to miss some flushing were perhaps needed. Also more lines of code, as the flush call is on a separate line usually.

I may be more sensitive here when it comes to code structure patters, so will not try to push/enforce my ideas here and just stick to having advertized the solution I find more elegant to counter the namespace change only, and not changing code logic for that, repeated again here:

namespace KF { // if not already in custom namespace
using TextStreamFunction = QTextStream& (*)(QTextStream&);
#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
constexpr TextStreamFunction endl = Qt::endl;
#else
constexpr TextStreamFunction endl = ::endl;
#endif
}
// use KF::endl now, or stick with endl if already in custom namespace

If you prefer moving away from endl instead, your choice, as you also do the work & patch, also do others seem okay with it as well, from what I saw by the reactions to the similar patches.

From me I am ok with David comment for this code it's not necessary to have a flush() many time.
Adding \n is enough for me.
Regards

Right, from a performance point of view, I believe this code was flushing far too much. In fact even now those calls to flush() are all unnecessary. Just one flush before closing the file is enough to be sure, or even without it, it will work (socket notifier says closed => stream flushes)