Goodbye, Q_FOREACH A porting guide to C++11 ranged for-loops
29.08.2016
Q_FOREACH (or the alternative form, foreach) will be deprecated soon, probably in Qt 5.9. Starting with Qt 5.7, you can use the QT_NO_FOREACH define to make sure that your code does not depend on Q_FOREACH.
You may have wondered what all the fuss is about. Why is there a continuous stream of commits going to into Qt replacing Q_FOREACH with C++11 ranged for-loops? And why does it take so many commits and several Qt versions to port away from Q_FOREACH? Can’t we just globally search and replace Q_FOREACH (a, b) with for (a : b) and be done with it?
Read on for the answers.
What is Q_FOREACH?
Q_FOREACH is a macro, added for Qt 4, that allows to conveniently iterate over a Qt container:
Q_FOREACH(int i, container)
doSomethingWith(i);
Q_FOREACH(const QString &s : functionReturningQStringList())
doSomethingWith(s);
It basically works by copying the second argument into a variable called QForeachContainer, and then iterating over it. I’m only mentioning this for two reasons: First, you will start seeing that internal QForeachContainer at some point in deprecation warnings (probably starting with Qt 5.9), and, second, yes, you heard correctly, it copies the container.
This copying has two effects: First, since the copy taken is essentially const, no detaching happens when iterating, unlike if you use the C++98 or C++11 alternatives:
for (QStringList::const_iterator it = container.begin(), end = container.end(); it != end; ++it) doSomethingWith(*it); for (const auto &s : container) doSomethingWith((*it);
In both cases the (explicit or implicit) calls to begin() and end() cause a non-const container to detach from shared data, ie. to perform a deep-copy to gain a unique copy of the data.
This problem is well-known and there are tools to detect this situation (e.g. Clazy), so I won’t spend more time discussing it. Suffice to say that Q_FOREACH never causes detaches.
Except when it does.
Q_FOREACH is Convenient^WEvil
The second effect of Q_FOREACH taking a copy of the container is that the loop body can freely modify the original container. Here’s a very, very poor implementation that takes advantage of this:
Q_FOREACH(const QString &lang, languages)
languages += getSynonymsFor(lang);
Of course, since Q_FOREACH took a copy, once you perform the first loop iteration, languages will detach from that copy in Q_FOREACH, but this kind of code is safe when using Q_FOREACH, unlike when you use C++11 ranged for-loops:
for (const auto &lang : languages)
languages += getSynonymsFor(lang); // undefined behaviour if
// languages.size() + getSynonymsFor(lang).size() > languages.capacity()
So, as we saw, Q_FOREACH is convenient—if you write code.
Things look a bit different if you try to understand code that uses Q_FOREACH, because you often can’t tell whether the copy that Q_FOREACH unconditionally takes is actually needed in any particular case, or not. A loop that plain falls apart if the container is modified while iterating is much easier to reason about than a Q_FOREACH loop.
And this brings us to porting away from Q_FOREACH.
Towards a Q_FOREACH-Free World
Things would be pretty simple if you could just globally search and replace Q_FOREACH (a, b) with for (a : b) and be done with it. But alas, it ain’t so easy…
We now know that the body of a Q_FOREACH loop is free to modify the container it’s iterating over, and don’t even for a minute think that all cases are so easy to recognize as the example with the languages above. The modification of the container may be several functions deep in the call stack originating from the loop body.
So, the first question you need to ask yourself when porting a Q_FOREACH loop is:
Does the loop body (directly or indirectly) modify the container iterated over?
If the answer is yes, you also need to take a copy and iterate over the copy, but as the nice guy that you are, you will leave a comment telling the future You why that copy is necessary:
const auto containerCopy = container; // doSomethingWith() may modify 'container' if ....
for (const auto &e : containerCopy)
doSomethingWith(e);
I should note that in cases where the container modification is restricted to appends, you can avoid the copy (and the detach caused by it) by using an indexed loop:
for (auto end = languages.size(), i = 0; i != end; ++i) // important: cache 'languages.size()'
languages += getSynonymsFor(languages[i]);
With that question answered, the next one is:
What are you actually iterating over?
Avoiding Detaching
If your container is a std:: container or QVarLengthArray, you replace the Q_FOREACH(a, b) with for (a : b) and you are done. Arguably, Q_FOREACH should never, ever have been used on such a container, since copying those always copies all elements (deep copy).
If your container is a const lvalue or a const rvalue, you can do the same. Const objects don’t detach, not even the Qt containers.
If your container is a non-const rvalue, simply store it in an automatic const variable, and iterate over that:
const auto strings = functionReturningQStringList();
for (const QString &s : strings)
doSomethingWith(s);
Semantically, this is exactly what Q_FOREACH did before, so this case is easiest to verify (the body could not possibly have modified the container, as it didn’t have access to it).
Last, not least, if your container is a non-const lvalue, you have two choices: Make the container const, or, if that doesn’t work, use std::as_const() (new in C++17, but easily implemented yourself, if required) or qAsConst() (new in Qt 5.7) to cast to const:
for (const QString &s : qAsConst(container))
doSomethingWith(s);
There, no detaches, no unnecessary copies. Maximum efficiency and maximum readability.
Conclusion
Here’s why you’ll want to port away from Q_FOREACH, ideally to C++11 ranged for-loops:
Q_FOREACHis going to be deprecated soon.- It only works efficiently on (some) Qt containers; it performs prohibitively expensive on all
stdcontainers,QVarLengthArray, and doesn’t work at all for C arrays. - Even where it works as advertised, it typically costs ~100 bytes of text size more per loop than the C++11 ranged for-loop.
- Its unconditionally taking a copy of the container makes it hard to reason about the loop.
Happy porting!
Thanks for the detailed explanation Marc.
And thank you for all your work to investigate & clean things up. It is appreciated.
Some of us don’t have the option to use C++11, especially those of us in the embedded space. Project leads, or funky OS requirements, sometimes prevent us from using what we want to use.
There is no way Q_FOREACH can’t expand into C+=11 ranged for() loop?
Qt 5.7+ requires a working C++11 compiler, and in Qt 5.6,
Q_FOREACHwill not be deprecated, so there’s no situation in which you’re faced with deprecatedQ_FOREACHand no C++11 compiler.It should be clear that
Q_FOREACHcannot expand into C++11 ranged-for, sinceQ_FOREACHcopies and C++11 ranged-for does not.You give this as a loop copying the source container:
const auto containerCopy = container; for (const auto &e : containerCopy) {}Isn’t that equivalent to
for (auto e : container) {}? The
fordeclaration (with neither const nor reference one) forces a copy to be made.It seems to be disputed where the copy is of ‘e’ or of ‘container’ (the latter is my reading of this page
https://msdn.microsoft.com/en-us/library/jj203382.aspx and also what seems to happen in GCC when I try this, but others suggest that only ‘e’ is a copy). There are probably edge cases in which this matters.
The element is copied, not the container. I don’t care about MSDN docs, what matters is the C++ standard, which says:
([stmt.ranged]/1). No copy is taken. Temporaries that are bound to
__rangehave their life-time extended for the duration of the life-time of the reference to which they are bound, but that does not constitute a copy).If a compiler copies the container, report a bug.
What if I define my FOREACH macro just coping the container and using the c++11 for-loop? Wouldn’t it work exactly like the old Q_FOREACH? In this case I prefer to be safe and define my macro than reason about code that maybe someone else wrote, and that is hard to reason about.
Marc,
I don’t know how to say this gently, so I’ll be rude This is idiocy. Unless you will personally port everything in the world that uses Q_FOREACH. Please do start with Krita, with about 1600 instances of it. Also, do stay around, because that port will, for sure, cause a host of bugs.
http://www.valdyas.org/fading/index.cgi/hacking/porting_redux.html
The “port” is trivial. Add
to one of your central header files.
I dispute your blog post, of course. They’re egoistic. You want libraries with new features and no deprecation. You cannot have both, unless the development resources spent on the library are increased with the complexity of the library. You never change the user interface of Krita? Are your users happy with a 2000-ish user interface? If you change the user interface, don’t they complain, too? Some?
I started out criticising Qt. For
QList, e.g. And you know why my “Effective Qt” blog series all but faded away? Because I started to fix the problems instead of writing about them. I suggest you do the same. Share in the maintenance of Qt instead of rudely demanding a no-deprecation policy from your upstreams. Come, take a peek behind the curtain. Take a walk in my shoes. You’ll stumble in my footsteps. etcppI guess you don’t know about Nimmy’s Summer of Code project then, where he basically spent a Krita/KDE slot on porting the Qt OpenGL QPainter engine to 3.0 core profile so it works again on OSX. Which I mentored. But that is actual useful work, so I guess you’re not interested in that — in contrast to things like deprecating q_foreach, which is just churn, costing people lost of time and delivering nothing useful in return.
Thanks for contributing to Qt, if only as a mentor. I do appreciate it.
Let me tell you two anecdotes that may make my position clearer:
Qt 1 had optional template support. You could declare containers the old-fashioned way with a macro. That was necessary, because and for as long as, not all target compilers supported templates sufficiently enough. In Qt 2, that way of defining a container was dropped. Would you like to work with this kind of 80s C++ API in 2016? No-one would like to work with such an API today! But back then, some Qt users were probably also writing insulting blog posts and calling Qt developers names: “It’s 2000, and my compiler still doesn’t support templates! F*ck. How dare you take away the macro-based Qt containers?!”. Should Qt have kept those macros for Qt 2? Qt 3? Qt 4? Qt 5, even? That would have been grist to the Copperspice’ guys’ mill, and rightfully so. No, the Trolls were right to drop that workaround as soon as all target compilers grokked templates well enough.
This debate here will look just as silly in 10-15 years as the Qt 1->2 container change debate did back in 2000.
Understand that my concern is the shape of Qt in 10+ years. We need an exit strategy from developments that are side-line to the core competencies of Qt and are more featurefully and/or efficiently implemented in C++ itself. Over the last 6 years, std C++ has overtaken Qt in almost all areas in which std C++ invested in new API: containers, multi-threading, smart pointers, iteration, algorithms, even file-system. And Qt can’t participate because it’s stuck in its own abstractions that differ so very slightly from the std ones.
Here’s the second anecdote: I tried to do better with
qAsConst(). We can’t requirestd::as_const(), yet, because not all target compilers have it. But I designed and implementedqAsConst()so it has exactly the same semantics (and implementation!) asstd::as_const(), making it trivial to migrate away from it in the future. But, as usual, as soon asqAsConst()was added, clever people wanted to make it work with rvalues, to make the non-const rvalue case “more convenient”. I’m happy to report that this attempt was thwarded, because it would have meant a) worse performance than what I suggest here and, more importantly b) porting overhead for users 10 years down the line, when Qt comes to depend on a version of C++ that containsstd::as_const(), and we deprecateqAsConst().Q_FOREACHwas a workaround for a missing ranged-for loop in C++. Now that we require ranged-for loops in the compiler, it’s time to phase out the workaround. IfQ_FOREACHhad been designed with the same goal of upstream compatibility (and it could, the ranged-for proposal is from 2006, IIRC, and even before thatBOOST_FOREACHexisted with roughly the same semantics as C++11 ranged for-loops), we wouldn’t have this discussion now. No, someone wanted it more convenient, and now 100000s of uses need to be reviewed to phase it out.There are tons of these examples (
QWeakPointer‘s support forQObjectlifetime tracking comes to mind), but Qt is getting better at this. We rejected a patch to add aQOptionallast year, e.g.Sorry, but from the purely pragmatic standpoint this is a terrible idea. In our codebase we have ~3000 uses of Q_FOREACH. We are ~3 active developers (none full-time). Let’s assume it takes me just one minute to port each use of Q_FOREACH to range-based for. That makes about 3 full work days of just sitting there and replacing Q_FOREACH by range-based for, for our whole team. That is like a complete sprint for just this change. Plus it will have zero effect on the actual problems our code base has — if we would all spend 3 days to clean those up, something _would_ actually improve.
This, instead, will just result in another pointless porting orgy (hello, KUrl/QUrl port) with no gain in maintainability, performance, or code quality whatsoever. It will just cause lots of hard-to-track bugs in unnecessary places.
Don’t do this. It’s changing things for some “greater good” nobody in reality cares about. Which is destroying exactly the pragmatism that makes Qt so much more pleasant to use than e.g. Boost.
No-one forces you to enable
-Wdeprecated. Even whenQ_FOREACHshould one day be removed from Qt, you can just copy the Qt 5 definition over and continue to use it.But Qt no longer will need to maintain an inferior solution.
APIs have but two choices: evolve and else rot. I guess we all agree we want Qt to be in the first category 🙂
Needing to maintain it, well, it’s like ten lines of code which I guess were not touched since 2005. So the maintenance burden is probably bearable. It seems quite likely that all persons ever maintaining Q_FOREACH combined didn’t spend as much time on it as it will take just our application to port away from it. The “it’s just deprecated, not removed” argument I don’t buy, sorry — if you didn’t want to remove it, you wouldn’t deprecate it. This is an announcement for removal.
Yes, I can copy it over and keep using it. But with the same reasoning, you can just leave it upstream … plus, “you can fork it” is a moot argument in free software, also in this form.
It hurts noone to leave it in and e.g. just not recommend to use it in the documentation. It hurts a lot of people to take it out. So why remove it? Yes, there is certainly code which does strange things with it. But: that code works. It was written and tested in 2004 by somebody who is long gone. You break that code by forcing us to touch it for no good reason. With certainity, the person touching it will not even be super careful, because that person knows she has to fix 3000 further occurences and cannot take twenty minutes to think about each.
There are applications which basically died from the fallout of exactly such changes. Just look at Okular and the KUrl/QUrl situation.
Yes, API has to be cleaned up sometimes. But there’s a big difference between cleaning up an old API which really needs to be cleaned up because it became hard to use from growth; and removing the little warts that hurt nobody and you only want to remove because they don’t look so pretty and modern on your website. Q_FOREACH completely falls in the second category.
Again: What are the arguments for removing Q_FOREACH? “It is evil because you can write unintuitive code with it”? That is an extremely bad argument, because this code _works right now_. It will break exactly if you force people to touch it by removing Q_FOREACH.
Please at least
git blamethe code before you make such unfounded assertions as “hasn’t been touched since 2005”.If it’s so easy to maintain as you say, then go ahead and maintain it. In your own codebase, where you control the compilers. Don’t ask of other people what you don’t even want to do yourself.
The reason for removing
Q_FOREACHis that there’s now a superior alternative in C++11 itself, and Qt 5.7+ requires this feature in the compiler, soQ_FOREACHbecomes a burden to maintain.Qt doesn’t have infinite development resources, so removing ballast to be able to concentrate on the core competencies is something that must be done now and then. We deprecated all the Qt algorithms that duplicated the STL ones, as soon as we could depend on full C++98 support. We couldn’t remove
Q_FOREACHfor 5.0, because we couldn’t depend on working ranged for-loop support in the compiler. We can’t define it to ranged for-loops, either, because that breaks users. So, we phase it out.The claim that Q_FOREACH is a burden to maintain for Qt and it’s too much to do with the finite development resources it has is ridiculous. It’s *ten lines of code*. But I know why you’re saying it; it is the only point which can be made in favour of its removal.
Let’s take a look: https://code.woboq.org/qt5/qtbase/src/corelib/global/qglobal.h.html#930 It’s 40 loc, of which 9 are comments, 4 are empty lines, and 4 are for the handling of
QT_NO_FOREACH. Do you understand this code?It has an obvious performance improvement. I’ll add it in the next week or so, even though I’m the one suggesting to port away from it. Can you spot it? Kevin? Boud? Sven?
No? See? It’s not “*ten lines of code*”. It’s a complex feature that needs maintaining.
10 lines here, 10 lines there. Many mickle makes a muckle.
I note the absence of replies to the question regarding the obvious performance improvement.
C’mon, it’s *ten lines of code*.’Or maybe it is a complex feature, after all?
Hint: it’s about avoiding a copy in one particular use-case.
Yes, I guessed as much.
Solution: https://codereview.qt-project.org/170073.
> It’s 40 loc, of which 9 are comments, 4 are empty lines, and 4 are for the handling of QT_NO_FOREACH.
The implementation of QT_FOREACH is just 14 functional (i.e., non-empty, non-comment) lines of code. The rest is trivial stuff such as “#define Q_FOREVER for(;;)” that costs nothing to maintain.
I don’t really feel like discussing whether it is too much work to maintain those — ok, as Kevin said — 14! lines of code. If you go into yourself, I’m confident you will agree that the maintenance burden is not why you want to remove it. You want to remove it because C++ offers a better option, and you want the worse alternative removed from your library. You don’t want to be frowned at by people saying “ugh, Qt, that maintains its own copy of the C++ standard library for no reason”. I understand that feeling.
But it doesn’t matter. You are working on a library used by hundreds of thousands of people. Your #1 task as a library author is to make it as painless to use for those people as possible. Your task is not to educate those people on how to write modern, efficient or pretty code!
You are a library, damnit. You added the macro, it is used millions of times across the world, you are obligated to keep it around. Forever. Projects like C++ understood this — once you add a feature, you are stuck with it forever. Even Microsoft understood this. Do you think anybody would still use C++ if they would remove features they don’t like any more every five years? Do you think anybody would still use x86 CPUs if they had decided “ah, no, that part of the instruction set is crap, let’s remove it” at some point?
Want an example? Look at Python. 6 years (!!) after the release of Python 3.0, almost 90% of stuff still used Python 2. (see e.g. https://blog.newrelic.com/2014/01/21/python-3-adoption-web-apps/)
And why? Because they, for example, replaced the previous strings by separate “string” and “byte array” objects. A change which is seemingly innocent (like the removal of Q_FOREACH might sound to you) and provides an advantage for future code written (much unlike the removal of Q_FOREACH, which future code can simply … not use). But still to this day large, important projects like twisted have not managed to port. A lot of people are very angry about this change. A lot of people moved to a different language which doesn’t do this kind of stuff. And remember, this was a change which actually made sense because it fixed API that was _really bad_. Do you want people to feel the same about Qt, without even improving anything?
If you start removing the Qt containers, things will happen. People will either fork them, or just stop using your library. I know for sure that I am not going to port any of KDE away from Q_FOREACH. Or QList. Or QVector. Then I’d rather just say “screw all that”, grab a glass of wine and sit in the sun, because that’s just a waste of my life. Think about it like this: assume we’d do a sprint and port all this. With any normal person (i.e. not somebody spending his life on making code look pretty according to some arbitrary definition of “pretty”) a conversation would go something like
“oh, cool you’re doing a sprint — what did you do, which nice new features did you add?”
— “oh, we ported away from Q_FOREACH”
“okay, maintenance work … I guess that makes the application faster, or easier to maintain?”
— “actually no, not really, it will probably introduce new issues both with performance and with functionality.”
“hm, why are you doing it then?”
— “because somebody decided it was prettier to do it like this.”
“oh.”
Please be pragmatic. It’s about finding the solution that causes the least pain for all of us together. And in this case that solution is simply keeping those 14 lines of code in that header file.
Actually, we’d rather NOT have this kind of “evolution”. See Boud’s blog post he linked to. Spending time porting code away from a heavily-used construct is just a pain and gains us nothing. We’d rather have a library that gets only bugfixes than a library that breaks our code with such major source-incompatible changes. And this deprecation *is* a source-incompatible change because any deprecated feature will eventually be removed, and then -Wno-deprecated will not help anymore either.
Let me correct you on that: you‘d rather not have this kind of evolution. If you want no evolution, then stick to Qt 5.6 LTS. Or, for that matter, 4.8.
Phasing stuff out is a necessary process to keep a software alive. A CI can only test so many different platforms before it grinds to a halt, developers can only maintain so much code until they make grave mistakes. You want Qt to gain new features, so something has to give. The Qt containers are essentially on life support. No-one added post-C++98-API, no-one made them use C++11 features inside. I tried, but it’s just so much a drain on developer resources better spent creating something new and faster than to play catch-up with a rapidly evolving upstream, that I stopped trying.
We dropped several supported platforms with 5.0. We dropped more with 5.7. We dropped Qt algorithms. We’ll drop
Q_FOREACH, we’ll drop Java-style iterators, and we’ll dropQList, maybe even all Qt containers, at some point. Every one of these changes hurts someone, probably more than it will hurt you or anyone else to copy theQ_FOREACHdefinition into your own project to keep using it. That doesn’t mean a library can just carry that cruft with it until eternity, even if Boud and you would very much like that.Sorry for reply year old topic. But I have a questions.
1. I don’t mind much about deprecation of Q_FOREACH, so just curiosity. Couldn’t Q_FOREACH be reimplemented on top of range-based for? To drop several lines. Like this one:
#define Q_FOREACH(variable, container) \
for (std::pair _container_(true, container); \
_container_.first; _container_.first = false) \
for (variable : _container_.second)
Same copy behaviour as original Q_FOREACH. std::pair has moving constructor. Some drawbacks I coudn’t see?
2. What I’m concerned is Qt containers. Especially QList. Qt containers has different method naming. Like rest of Qt classes. Replacing them with std containers would lead to ugly naming mix. Also replacing containers in existing code would lead to replacing a lot of method calls. Some of them even couldn’t be replaced with one method call, e.g. endsWith, some should be replaced with function calls, e.g. count(x). And yes, sometimes it very nice to have COW behaviour (e.g. changes to copy is unlikely but possible).
But mostly I would miss QList. There is no similar class in std, am I right? Most similar to QList is std::vector<const std::unique_ptr>. But it would be less effective, I believe container control of reallocation of pointer array needs just something like memmove, but reallocation of unique_ptr would lead to iteration calls of moving constructors and destructors. Or maybe I just wrong about QList implementation.
Oh, sorry, parser eat some part of code.
#define Q_FOREACH(variable, container) \
for (std::pair<bool, decltype(container)> _container_(true, container); \
_container_.first; _container_.first = false) \
for (variable : _container_.second)
“we’ll drop QList”
Can you give me an approximate date? Because I’m still using it heavily, even in new code…
QList has not been deprecated in fine (this blog post is quite old and pre-date Qt6).
QList and QVector are now the same thing in Qt6, see here: https://www.qt.io/blog/qlist-changes-in-qt-6
You know, this argument is eerily recurrent. I recall back in the late 70’s the hew and cry when ANSI had the audacity to remove COBOL’s ALTER verb. (For you broomtails out there, ALTER was a way to write self-modifying COBOL code. The mind boggles at the mere thought of such an abomination… but there it was.) I remember reading in the Letter To The Editor in InfoWorld about how some guy here had “thousands of lines of ALTER code”, and how it would take years to rewrite all the code, blah, blah, blah, yadda, yadda…. Yet nobody in their right mind would even consider writing self-modifying code nowadays. (It’s likely nobody in their right mind would consider writing COBOL nowadays…but I digress.)
The state of the art moves on. We either move along with it, or become dinosaurs. Yes, updating code containing obsolete macros is a PITA; I’ve had to do it myself transitioning from Qt4 to Qt5. It’s part of maintaining an application across generations of compilers/libraries/development environments.
I suppose you could always stay with the current version of Qt you’re using.
Great read, Mark! Thanks many!
I think this is a horrible idea to deprecate it! Deprecation means that it’s going to be removed with Qt 6. I just checked for a few plasma-workspace modules and we easily get to a few thousand usages. It’s not a straight-forward port as you outlined in your blog post. Sven is optimistic with 1 min per usage. I am seeing this more realistic with 2 to 10 min per usage. If we have only one regression due to it we get quickly into hours to figure out why that now crashed.
This makes me very afraid of a possible Qt 6 port. Even if it doesn’t get removed the hundreds of deprecation warnings will also make the compile output just useless.
Please reconsider what this will cost to the Qt ecosystem to deprecate it.
Most are rather straight-forward. Some are hard. You can always leave the hard ones in and copy the definition of
Q_FOREACHto your project, sans the deprecation warnings.Do I get this correctly: you suggest all projects to just copy Q_FOREACH into their project instead of leaving it in Qt? That’s just: wow!
Also it’s nonesense as a suggestion the way you just did. I need to look through each of them and for each of them read through what the loop body does and evaluate whether it’s a potential hard or easy one. That’s where the time is spent. And that time is spent for every foreach loop we have in the code base – whether it’s easy to port or hard.
We cannot assume it’s trivial to port thus a human needs to look at each one and think. That’s why it’s costly and pointless as others have pointed out here.
To me that’s porting for the sake of porting. That’s nothing I’m interested in. No user of my software will benefit from it – quite the opposite. They will suffer from it. Because we spend our spare resources doing useless porting and risking breakage. Breakage which is difficult to detect in a legacy code base with potentially no test coverage and being in code areas not run my the main devs any more (guess what: in KWin most foreach loops are in the X11 code base, the new Wayland code base uses the new for loop). How is one supposed to port that with a good feeling, when one knows that doing a mistake in this boring work can have severe consequences for the users? All for what? Sorry I don’t get it.
I can only highly recommend to reconsider whether it’s worth it.
Your arguments are true—of any deprecation. Extending this argument to its logical consequence just means that we should never have had a Qt 2, but should have stayed on Qt 1.x forever. That’s just nonsense. Good API survives (you can probably still take a Qt 1 paintEvent() and it would compile in Qt 5), bad API is phased out. This is how QList looked like if it wasn’t for deprecation:
#define QListMdeclare(type) class Q_EXPORTM QListM(type) : public QGList { public: QListM(type)() {} QListM(type)( const QListM(type) &l ) : QGList(l) {} ~QListM(type)() { clear(); } QListM(type) &operator=(const QListM(type) &l) { return (QListM(type)&)QGList::operator=(l); } uint count() const { return QGList::count(); } bool isEmpty() const { return QGList::count() == 0; } bool insert( uint i, const type *d){ return QGList::insertAt(i,(GCI)d); } void inSort( const type *d ) { QGList::inSort((GCI)d); } void append( const type *d ) { QGList::append((GCI)d); } bool remove( uint i ) { return QGList::removeAt(i); } bool remove() { return QGList::remove((GCI)0); } bool remove( const type *d ) { return QGList::remove((GCI)d); } bool removeRef( const type *d ) { return QGList::removeRef((GCI)d); } void removeNode( QLNode *n ) { QGList::removeNode(n); } bool removeFirst() { return QGList::removeFirst(); } bool removeLast() { return QGList::removeLast(); } type *take( uint i ) { return (type *)QGList::takeAt(i); } type *take() { return (type *)QGList::take(); } type *takeNode( QLNode *n ) { return (type *)QGList::takeNode(n);} void clear() { QGList::clear(); } int find( const type *d ) { return QGList::find((GCI)d); } int findNext( const type *d ) { return QGList::find((GCI)d,FALSE);} int findRef( const type *d ) { return QGList::findRef((GCI)d); } int findNextRef( const type *d ){ return QGList::findRef((GCI)d,FALSE);} uint contains( const type *d ) const { return QGList::contains((GCI)d); } uint containsRef( const type *d ) const { return QGList::containsRef((GCI)d);} type *at( uint i ) { return (type *)QGList::at(i); } int at() const { return QGList::at(); } type *current() const { return (type *)QGList::get(); } QLNode *currentNode() const { return QGList::currentNode(); } type *getFirst() const { return (type *)QGList::cfirst(); } type *getLast() const { return (type *)QGList::clast(); } type *first() { return (type *)QGList::first(); } type *last() { return (type *)QGList::last(); } type *next() { return (type *)QGList::next(); } type *prev() { return (type *)QGList::prev(); } void toVector( QGVector *vec )const{ QGList::toVector(vec); } private: void deleteItem( GCI d ) { if ( del_item ) delete (type *)d; } }That’s all you ever want from a container. It’s there. But the product improves if stuff like that gets killed off. You may not see the benefit immediately, but you will see it:
First, from a smaller executable and faster code (I once heard that
Q_FOREACHis too convoluted for the optimizer to figure out and that C++11 ranged for-loops thus re-enable the autovectoriser; if had found the source, I’d’ve included it in the article).And second, don’t underestimate the kind of nonsense that you will find when you review an existing program’s loops. Loops are used instead of +=, etcpp. More likely than not, you will find some bugs in your code, too, and the net bug count will be negative (more bugs found than caused). I know I did, in Qt. Code you never look at has bugs. Period. The more so if you’re afraid of touching it 🙂
I’d like to point out that there’s no consensus (or decision) to formally deprecate it (as in: Let the compiler warn about it’s usage).
Marc, is there a slot already at the qtcon this weekend to discuss this proposal?
A “looks great, […] we should deprecate Q_FOREVER at the same time” from Lars is a pretty strong indication that a decision has been made, but ok, feel free to open up that can of worms again.
Sadly, I again can’t make it to QtCon this year, but I’m not convinced that a committee meeting will help here. There’s those who are so shell-shocked from the Qt 3 → 4 transition that they deny right-of-existence for any kind of source-incompatible change, and then there’s those who think that Qt has grown too fat and needs to lose some weight. I’ve seen the
wip/litebranch fly around, and see Lars work on compile-time features in modules other than QtBase, and I think if you put two and two together here…I guess there’s room for middle-ground here. We continue to deprecate stuff, to indicate to new users that these APIs should not be used in new code, but we don’t remove deprecated API in Qt 6, but keep it around, deprecated, and unmaintained (CI runs with stuff deprecated before Qt 6.0 compiled out). In return, users stop insisting on having a
-Wdeprecated-clean build without any investment of their own, and provide patches if their favourite deprecated API breaks with new compilers. In return, we promise to actually take these patches in (instead of arguing “it’s deprecated API, why should I care?”).> A “looks great, […] we should deprecate Q_FOREVER at the same time”
> from Lars is a pretty strong indication that a decision has been made, but ok,
> feel free to open up that can of worms again.
He said that about Q_FOREVER. Not Q_FOREACH. Q_FOREVER is indeed trivial to port for the 5 uses it has by just one line of sed. Feel free to remove that. The only voice in that patch regarging removal of Q_FOREACH is
> I am fine with replacing uses of foreach in *our* .cpp.
> I am not really ok with forcing that decision on customer code.
which I fully agree with.
You said:
> This debate here will look just as silly in 10-15 years as the Qt 1->2 container
> change debate did back in 2000.
Exactly. And then the time has come where you can delete the API — when nobody is using it any more.
If you would have looked even superficially, you would have noticed that “looks great” is a comment about the patch. The patch which happens to deprecate
Q_FOREACH(but notQ_FOREVER).But all the outcry is over the deprecation. I’m fine with keeping
Q_FOREACHin the header, deprecated, and unmaintained(!), until eternity. See my reply to Kai’s comment. I’m not fine with users who insist on a-Wdeprecated-clean build while at the same time refusing to port their code to non-deprecated API. Either refuse to port, or have a-Wdeprecated-clean build. You can’t have both. Not with C++ and not with Qt. If you usestd::strstream, fine. You can. But don’t complain about the deprecation warning. If you useQ_FOREACH, fine, continue to do so, but don’t complain about the deprecation warning.Under these conditions, I’d be fine with keeping deprecated APIs in (unmaintained) for 10-15 years.
I suggest we take this discussion about deprecation and removal of API to the ML now. I’ve posted an anchor mail. Please join there.
> if your container is a non-const lvalue, you have two choices:
> Mark the variable const, or, if that doesn’t work,
> use std::as_const() or qAsConst()
Marking the variable as const does not prevent detaching. Using qAsCont is always needed.
At least this is what I see when debug tracing.
Maybe post a small demo that shows this?
The 1st loop causes detaching, because iterator begin() is called.
The 2nd loop does not, because const_iterator begin() const is called.
Tested undet Visual 2015 update 3
You said:
I don’t see where you’re marking the variable const. Ahh, no, not the loop variable. The container. Will clarify in the post. Thanks!
..and the outcry about porting cost continues.
If any of you could just bother to look for *smarter* ways for porting, like clang’s tidy/modernize tool. Writing a clangs based tool for converting Q_FOREACH shouldn’t take more than a week for one dev, and everyone would benefit. Porting cost would drop to <1 day.
That’s a good idea, and the Qt developers are thinking about it for Qt 6.
But afaik, the problem is tracing calls across TUs, so for the (likely) case where the loop contains a call to an out-of-TU function, a clang-based modernizer couldn’t give a go for porting to ranged-for, because that out-of-TU call could re-enter into the current TU and cause a modification to the container, right?
But for simple cases, this could work, yes.
I mostly managed to port away from Q_FOREACH in KDE Partition Manager. I think I’ll replace remaining Q_FOREACH once I can depend on Qt 5.7 with qAsConst
KDevelop tooltips really helped me to do the port (it quickly shows whether rvalues are constant or non constant, etc) and it was much easier than e.g. many other ports when porting to KF5.
Thanks for your post, Marc! I’ve seen bugs myself that were due to the container being modified inside the Q_FOREACH loop.
I think that more than 99% of all Q_FOREACH uses are easy to port. Code that is not easy to port is code that is hard to reason about and that might actually contain bugs that have not been found yet.
Deprecation will prevent people from (often unknowingly) write such code, so I think that it’s a good idea.
I wonder whether Qt could not provide a different semantics for qAsConst than std::as_const? For rvalues, return a (const) copy? So that you don’t have to think about rvalues when writing the code. Alternatively, if you detect a Qt type, return a const copy (to keep COW quiet), and otherwise return a non-const copy (to enable move construction)?
There’s a reason why
std::as_constfor rvalues is explicitly= delete. I don’t know the reason. but I guess it’s a combination of the lack of life-time extension when the rvalue is piped through a function first, leading to too-early destruction of the container, and the lack of interest in trading perceived convenience for very real performance degradation in vanilla C++. Because your idea would even cause aQVarLengthArrayorstd::arrayto be deep-copied/deep-moved, whereas if you are forced to store the temporary in an lvalue, you will get neither copies nor moves, neither deep nor shallow, due to RVO.That’s
std::as_const.As for why
qAsConstcan’t go beyond that, see my other comment, and https://codereview.qt-project.org/167560.TL;DR: If we let a
QFoogrow beyond the correspondingstd::foo, we’re stuck with it for eternity. Happened withQVector, which is COWed,QSharedPointer, which isn’t exception-safe on construction,QWeakPointer, which has another mode if the payload is aQObject, etc.Some people think it’s a good idea to take a standardised component, implement it with a Q in front (poorly, usually, compared to what a std implementor could afford to deliver), and then add new stuff on top that the standard doesn’t have.
But I think that’s a terrible idea: It binds development resources better spent elsewhere, it usually means that after the initial implementation, when the standard evolves, the Qt copy is falling behind, while at the same time making it but impossible to transition to the original, because users have become so invested into that delta between Q and std that they are very, very unwilling to let go.
I hope that from now on, when we add a copy of a std component to Qt, we do so with all the guarantees that the standard demands of an implementation, as much as possible, and with a clear message that as soon as all of Qt’s target compilers support the standard component, Qt will transition to it, and expect its users to do so, too. And with rvalue
qAsConstand the rejection ofQOptionalandQFunction, I think that the people that matter in Qt have come to the same conclusion.>> QVector, which is COWed
What’s the problem with that? the fact that all Qt containers are COWed is THE reason why they are greatly convenient to use.
If you had read the comment you are quoting from, you would know that the problem is that COW in a Q|std vector is non-standard behaviour, making it harder than necessary to port to
std::vectorinstead.Instead, we’re now afflicted with an unmaintained container (many, actually) we can’t get rid of, because the users are thick as thieves with the “Qt” containers, which really are only poor re-implementations of the STL containers with a Q stuck at the front. If you are a user of Qt, that should make you worried. If it doesn’t, you haven’t understood the problem yet.
>> If you are a user of Qt, that should make you worried
I don’t understand this statement. I have a long experience:
I have been using STL since its first availabilities, +20 years ago.
But since I know Qt (8 years), I mainly use Qt containers, because of their design (COW, more complete API, good naming).
The keyword is “unmaintained”.
Do anyone can suggest a regex for easy “foreach/Q_FOREACH -> C++11 ranged-for-loop” find and replace in all the source code? Thank you
You can’t do this safely, because if a foreach loop modifies the container, the code will likely crash when using a for loop instead (unless you change the inner code, of course). While it is safe to do so with foreach (because of cow).
Thank you for the tip. Anyway I’m sure I never modify the container.
To simplify porting, would it be possible to temporarily change the macro to return a const container? If that should be possible, the idea is, that then the compiler would highlight all hard-to-port occurrences as errors. After porting them manually, the rest could be ported automatically.
Patrick: the Q_FOREACH macro doesn’t “return” a container, but your comment gave me an idea: to catch all the cases where the container isn’t const, maybe one could hack Q_FOREACH to give an error in that case, e.g. by calling a dummy function that takes a const container as argument.
But we’re discussing (in KDAB) writing a clang plugin for this, it’s probably a better solution.
>> use std::as_const() or qAsConst() (new in Qt 5.7, but easily implemented yourself, if required) to cast to const
It should at least be mentioned that as_const is a c++17 feature, which needs additional stuff in the .pro file unless one is using msvc2015 or later (not sure which versions of Qt support “CONFIG += c++17” ?)
Would it be possible to share an implementation of qAsConst for Qt 5.6, since you call the implementation easy ?
I’ve added (C++17) to
std::as_const()now, thanks.An implementation of
std::as_const()is at http://en.cppreference.com/w/cpp/utility/as_constWhy not leave the opportunity to use Q_FOREACH with rvalue containers?
Is there a good reason to rewrite
Q_FOREACH(const QString &s : functionReturningQStringList())
doSomethingWith(s);
as
const auto strings = functionReturningQStringList();
for (const QString &s : strings)
doSomethingWith(s);
?
The second variant has the following drawbacks:
1) code rewriting
2) extra variable
3) one more line of code.
Would it work to replace all the method signatures inside Qt to return const containers?
From a quick check, this works just like taking a temporary const copy of the container without the need to do that explicitly every time.
I.e. changing
QStringList QString::split(const QString &sep, SplitBehavior behavior = KeepEmptyParts, Qt::CaseSensitivity cs = Qt::CaseSensitive) const
to
const QStringList QString::split(const QString &sep, SplitBehavior behavior = KeepEmptyParts, Qt::CaseSensitivity cs = Qt::CaseSensitive) const
If this works, that would considerably decrease the risk of unintentionally cause a detach. I can’t see any undesired side effects, but I am almost sure I forgot something, since the solution seems to be too easy that you wouldn’t have considered it already.
Making returned containers const inhibits move semantics, because const rvalues do not bind to the mutable rvalue references that move constructors and move assignment operators use.