kfunk added a comment.
In https://phabricator.kde.org/D3987#174960, @mkoller wrote:
> I just found out that this change introduced wrong replacements.
It did not, the changes are correct, though they're probably not what
everyone expects.
Please read through the comments in t
mkoller added a comment.
I just found out that this change introduced wrong replacements.
The changes in frameworks/kimageformats/src/imageformats like the following
are incorrect, since the return type is not a pointer but an enum flags type,
so it should still be 0, not nullptr.
@@ -
kossebau closed this revision.
REPOSITORY
R280 Prison
REVISION DETAIL
https://phabricator.kde.org/D3987
To: kfunk, #frameworks, dfaure, kossebau
Cc: skelly, kossebau, dfaure, graesslin
kossebau accepted this revision.
kossebau added a comment.
This revision is now accepted and ready to land.
In https://phabricator.kde.org/D3987#96434, @kfunk wrote:
> In https://phabricator.kde.org/D3987#96418, @kossebau wrote:
>
> > In https://phabricator.kde.org/D3987#95858, @kfunk
kfunk added a comment.
In https://phabricator.kde.org/D3987#96418, @kossebau wrote:
> In https://phabricator.kde.org/D3987#95858, @kfunk wrote:
>
> > Closing. This Diff refactored the code technically correct.
>
>
> Technically correct, as in: it builds.
> But semantically it
kossebau added a comment.
In https://phabricator.kde.org/D3987#95858, @kfunk wrote:
> Closing. This Diff refactored the code technically correct.
Technically correct, as in: it builds.
But semantically it is incorrect and a regression when it comes to flags,
especially as high l
kfunk added a comment.
Closing. This Diff refactored the code technically correct.
If you want to replace `MyFlags flags = nullptr` by `... = {}` please do so
in a follow-up patch/review.
REPOSITORY
R280 Prison
REVISION DETAIL
https://phabricator.kde.org/D3987
To: kfunk, #framework
dfaure added a comment.
The signature of KUrl::queryItems is
queryItems(const QueryItemsOptions &options = 0),
i.e. = {}, so the caller should just not pass any value ;-)
But otherwise I agree, being explicit on the calling side is the best option.
REPOSITORY
R280 Prison
REVISION
skelly added a comment.
It's true that QFlags has a ctor taking a nullptr. The bindings-related
problem was in our handling of enums by prepending a namespace, so we would end
up with Qt::nullptr. I've fixed that in ECM, and also fixed handling of = {}
for various types too.
So, at leas
kossebau added a comment.
So seems everyone so far agrees that `{}` is best for the default value.
What about the passed argument case? Let's look at the two respective samples:
NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), {},
{});
QCOMPARE(QStringLis
skelly added a comment.
In https://phabricator.kde.org/D3987#78468, @dfaure wrote:
> {} sounds like the best solution to me.
I think that should be used instead of nullptr anyway.
void foo(const QModelIndex& idx = {});
is obviously better than
void foo(const QModelIn
dfaure added a comment.
{} sounds like the best solution to me.
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, graess
kossebau added a comment.
Seems the nullptr for QFlags values also breaks Python binding generation:
> Attempting to build the python bindings now fails because our scripting to
> generate sip files (or sip itself - I didn't investigate further because
it
> seems obvious that the
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 remov
kossebau added a comment.
In https://phabricator.kde.org/D3987#78426, @kfunk wrote:
> In https://phabricator.kde.org/D3987#78421, @kossebau wrote:
>
> > void foo(T* t);
> > void foo(QFlags flags);
> >
>
>
> Both `foo(0)` & `foo(nullptr)` would call `void foo(T* t)`, no? =
kfunk added a comment.
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
kfunk added a comment.
In https://phabricator.kde.org/D3987#78421, @kossebau wrote:
> Another possible issue with using `nullptr`as value for flags: it might
break calling methods with overloads when once a pointer type and once a flags
type is used. Not seen, but there is a risk.
>
kossebau added a comment.
Another possible issue with using `nullptr`as value for flags: it might break
calling methods with overloads when once a pointer type and once a flags type
is used. Not seen, but there is a risk.
void foo(T* t);
void foo(QFlags flags);
REPOSITORY
R280 P
kossebau added a comment.
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
dfaure added a comment.
I agree. But it's the default value anyway, so why not remove it completely,
thus making everyone happy?
REPOSITORY
R280 Prison
REVISION DETAIL
https://phabricator.kde.org/D3987
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To:
kossebau added a comment.
> This is what we are using now and were using before. Seeing
MyFlags(nullptr) may look odd at first, but it's totally fine.
I agree that it is fine from a compiling POV. But from a human-reading-code
POV having a nullptr (thus a pointer) being assigned to a bit
kfunk added a comment.
In https://phabricator.kde.org/D3987#78257, @kossebau wrote:
> Seems the porting script had a few wrong matches, where 0 values for enums
were misinterpreted as pointer:
>
> E.g.
>
> @@ -1015 +1015 @@ void KWindowSystemPrivateX11::setShowingDesktop(bool
s
kossebau requested changes to this revision.
kossebau added a reviewer: kossebau.
kossebau added a comment.
This revision now requires changes to proceed.
To have this somehow flagged as broken I set it now to "Request Changes".
Please close again once there is a new separate diff opened.
REP
kossebau reopened this revision.
kossebau added a comment.
This revision is now accepted and ready to land.
Seems the porting script had a few wrong matches, where 0 values for enums
were misinterpreted as pointer:
E.g.
@@ -1015 +1015 @@ void KWindowSystemPrivateX11::setShowingDesk
kfunk added a comment.
In https://phabricator.kde.org/D3987#77726, @dfaure wrote:
> In https://phabricator.kde.org/D3987#77725, @kfunk wrote:
>
> > Want me to do a `s/Q_NULLPTR/nullptr/`, too?
>
>
> This would make things consistent and avoid confusing future contributors
about
dfaure added a comment.
In https://phabricator.kde.org/D3987#77725, @kfunk wrote:
> Want me to do a `s/Q_NULLPTR/nullptr/`, too?
This would make things consistent and avoid confusing future contributors
about which one to use, so I'd say yes.
REPOSITORY
R280 Prison
REVISION DE
kfunk added a comment.
Hmm, one thing I forgot to ask:
Want me to do a `s/Q_NULLPTR/nullptr/`, too?
REPOSITORY
R280 Prison
REVISION DETAIL
https://phabricator.kde.org/D3987
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: kfunk, #frameworks, dfau
This revision was automatically updated to reflect the committed changes.
Closed by commit R280:42ae697a1afe: Use nullptr everywhere (authored by kfunk).
CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D3987?vs=9785&id=10206#toc
REPOSITORY
R280 Prison
CHANGES SINCE LAST UPDATE
https://
kfunk updated the summary for this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D3987
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: kfunk, #frameworks, dfaure
Cc: dfaure, graesslin
dfaure accepted this revision.
dfaure added a reviewer: dfaure.
dfaure added a comment.
This revision is now accepted and ready to land.
Change it all.
One note about "what is a public header" -- the rule is that any private
header (i.e. not installed) is called _p.h
The rule isn't 100%
kfunk added inline comments.
INLINE COMMENTS
> graesslin wrote in job.h:50
> Question: is this change API and ABI stable?
Definitely ABI stable, since default arguments are not part of the function
signature.
I'm not aware this could break API compat either (well, only in case the
compiler co
graesslin added inline comments.
INLINE COMMENTS
> job.h:50
> KIOCORE_EXPORT QString buildHTMLErrorString(int errorCode, const QString
> &errorText,
> -const QUrl *reqUrl = 0, int method = -1);
> +const QUrl *reqUrl = nullptr, int method = -1);
>
Question: is this change API
kfunk updated the summary for this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D3987
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: kfunk, #frameworks
kfunk updated the summary for this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D3987
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: kfunk, #frameworks
kfunk created this revision.
kfunk added a reviewer: Frameworks.
kfunk set the repository for this revision to R241 KIO.
Restricted Application added a project: Frameworks.
REVISION SUMMARY
The full patch (all Frameworks ported to using nullptr instead of null
literals) changes around 9000 line
35 matches
Mail list logo
