D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[David Faure]

dfaure added a comment.


  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)

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R269:d08e77529bd1: endl is Qt:: namespaced in qt5.15. Port to 
\n and flush when QTextStream is not… (authored by mlaurent).

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26320?vs=72512=73177

REVISION DETAIL
  https://phabricator.kde.org/D26320

AFFECTED FILES
  tools/bluezapi2qt/CppGenerator.cpp
  tools/bluezapi2qt/XmlGenerator.cpp

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent added a comment.


  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

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent added a comment.


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

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent added a comment.


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

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[David Faure]

dfaure added a comment.


  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 :)

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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 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...

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent added a comment.


  nm  -gDC libQt5Core.so.5.15.0 | grep endl
  00336680 T endl(QTextStream&)
  00336500 T QTextStreamFunctions::endl(QTextStream&)
  003361e0 T Qt::endl(QTextStream&)

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Friedrich W. H. Kossebau]

kossebau added a comment.


  @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`?

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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 (QTextStream ) { return Qt::endl(s); }
#ELSE
QTextStream (QTextStream ) { 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...)

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent updated this revision to Diff 72512.
mlaurent added a comment.


  more \n merge

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26320?vs=72511=72512

BRANCH
  port_endl_qt5.15 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26320

AFFECTED FILES
  tools/bluezapi2qt/CppGenerator.cpp
  tools/bluezapi2qt/XmlGenerator.cpp

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent updated this revision to Diff 72511.
mlaurent added a comment.


  merge \n

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26320?vs=72443=72511

BRANCH
  port_endl_qt5.15 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26320

AFFECTED FILES
  tools/bluezapi2qt/CppGenerator.cpp
  tools/bluezapi2qt/XmlGenerator.cpp

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent added a comment.


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

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Friedrich W. H. Kossebau]

kossebau added a comment.


  @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 (QTextStream );
// [...]

} // 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)

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> CppGenerator.cpp:61
>  writeCopyrightHeader(stream);
> -stream << "#pragma once" << endl << endl;
> -stream << "#include " << endl << endl;
> -stream << "class QDBusObjectPath;" << endl << endl;
> -stream << "namespace BluezQt" << endl << "{" << endl << endl;
> -stream << "class " << className << ";" << endl << endl;
> -stream << "class " << className << "Adaptor : public 
> QDBusAbstractAdaptor" << endl << "{" << endl;
> -stream << "Q_OBJECT " << endl;
> -stream << "Q_CLASSINFO(\"D-Bus Interface\", \"" << 
> interface.name() << "\")" << endl;
> +stream << "#pragma once" << "\n" << "\n";
> +stream << "#include " << "\n" << "\n";

I'd either merge into the line or at least use '\n'.

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent added a reviewer: dfaure.

REPOSITORY
  R269 BluezQt

REVISION DETAIL
  https://phabricator.kde.org/D26320

To: mlaurent, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26320: endl is Qt:: namespaced in qt5.15. Port to \n and flush when QTextStream is not on a QFile

[Laurent Montel]

mlaurent created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mlaurent requested review of this revision.

REVISION SUMMARY
  Port endl to \n

REPOSITORY
  R269 BluezQt

BRANCH
  port_endl_qt5.15 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26320

AFFECTED FILES
  tools/bluezapi2qt/CppGenerator.cpp
  tools/bluezapi2qt/XmlGenerator.cpp

To: mlaurent
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns