[Differential] [Commented On] D3987: Use nullptr in all Frameworks (just diff in KIO shown here)

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In https://phabricator.kde.org/D3987#78427, @kfunk wrote:
  
  > In https://phabricator.kde.org/D3987#78420, @kossebau wrote:
  >
  > > In https://phabricator.kde.org/D3987#78383, @dfaure wrote:
  > >
  > > > I agree. But it's the default value anyway, so why not remove it completely, thus making everyone happy?
  > >
  > >
  > > What do you mean by "remove"? In the samples from a few comments above, the `0` (or now `nullptr`) is used as (default) value for an argument:
  > >
  > >   +    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), nullptr, nullptr);
  > >   +    explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
  > >   +                             const QString &text = QString(), Qt::WindowFlags flags = nullptr);
  > >   +    QCOMPARE(QStringList(queryUrl.queryItems(nullptr).keys()).join(", "),
  > >   +                               KLocale::TimeFormatOptions timeOptions = nullptr,
  > >
  > >
  > > How could that be removed here? Unless you mean replacing with the default constructor of the flags type? That might be even better for human readers at least, agreed :) Though not sure how easy that fix-up is, but maybe the clang  artist knows what to do?
  >
  >
  > `Qt::WindowFlags flags = {}`, or
  >  `Qt::WindowFlags flags = Qt::WindowFlags()`
  
  
  "What to do" was about how to create a diff automatically :)
  
  Have to say, personally I am used to the `0` to denote a bitmask without any bit set, and would assume quite some other people as well. Not sure if `{}` would be an improvement for the human reader, being another pattern to learn.
  
  But assuming we want to avoid `0` as value for QFlags-based types, when comparing
  
    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), {}, {});
    explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
                             const QString &text = QString(), Qt::WindowFlags flags = {});
    QCOMPARE(QStringList(queryUrl.queryItems({}).keys()).join(", "),
                               KLocale::TimeFormatOptions timeOptions = {},
  
  with
  
    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), NET::Properties(), NET::Properties2());
    explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
                             const QString &text = QString(), Qt::WindowFlags flags = Qt::WindowFlags());
    QCOMPARE(QStringList(queryUrl.queryItems(KUrl::QueryItemsOptions()).keys()).join(", "),
                               KLocale::TimeFormatOptions timeOptions = KLocale::TimeFormatOptions(),
  
  it might be nice to use `{}` for default values, as the type is given already, and the explicit constructor for arguments when calling methods, as it adds information in the code itself.
  
  My 2 cents. In the end I was okay with `0`, just am unhappy with `nullptr` being used for QFlags values, for the reasons given :)

REPOSITORY
  R280 Prison

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, #frameworks, dfaure, kossebau
Cc: kossebau, dfaure, graesslin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170118/337d7d45/attachment-0001.html>