| lubot_ | [telegram] <RikMills> As a note, rebuilding clazy in a PPA the build time tests fail in the same fashion | 09:01 |
|---|---|---|
| lubot_ | [telegram] <mitya57> No ideas about clazy, but I will upload fixed veusz today. | 09:09 |
| lubot_ | [telegram] <RikMills> @tsimonq2 @mitya57 also same fail seen if I build current debian/ubuntu clazy on KDE Neon with their Qt 5.15.8 | 10:05 |
| lubot_ | [telegram] <RikMills> i.e. jammy base | 10:06 |
| lubot_ | [telegram] <mitya57> According to test history, it _is_ related to Qt 5.15.8 somehow. | 10:08 |
| lubot_ | [telegram] <mitya57> Maybe https://code.qt.io/cgit/qt/qtbase.git/commit/?id=747800675044bc1782b9bc9b97cea2cc2ef2c06f is somehow related? | 10:08 |
| lubot_ | [telegram] <RikMills> Reported: https://bugs.kde.org/show_bug.cgi?id=464441 | 10:10 |
| lubot_ | [telegram] <mitya57> Thanks! | 10:12 |
| lubot_ | [telegram] <RikMills> Would that imply that the test is defunct, or the test case used is at least? (re @mitya57: According to test history, it _is_ related to Qt 5.15.8 somehow. | 10:41 |
| lubot_ | [telegram] <RikMills> Maybe https://code.qt.io/cgit/qt/qtbase.git/commit/?id=747800675044bc1782b9bc9b97cea2cc2ef2c06f is somehow related?) | 10:41 |
| lubot_ | [telegram] <RikMills> c++ foo is failing me ATM | 10:41 |
| lubot_ | [telegram] <mitya57> I think clazy no longer generates the warning it needs to generate, maybe because it was relying on some QVector implementation detail that changed now. But that's just a hypothesis. | 10:42 |
| lubot_ | [telegram] <RikMills> ``` | 10:44 |
| lubot_ | [telegram] <RikMills> # reserve-candidates | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> Finds places that could use a `reserve()` call. | 10:44 |
| lubot_ | [telegram] <RikMills> Whenever you know how many elements a container will hold you should reserve | 10:44 |
| lubot_ | [telegram] <RikMills> space in order to avoid repeated memory allocations. | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> #### Trivial example missing reserve() | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> QList<int> ages; | 10:44 |
| lubot_ | [telegram] <RikMills> // list.reserve(people.size()); | 10:44 |
| lubot_ | [telegram] <RikMills> for (auto person : people) | 10:44 |
| lubot_ | [telegram] <RikMills> list << person.age(); | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> Example where reserve shouldn't be used: | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> QList<int> list; | 10:44 |
| lubot_ | [telegram] <RikMills> for (int i = 0; i < 1000; ++i) { | 10:44 |
| lubot_ | [telegram] <RikMills> // reserve() will be called 1000 times, meaning 1000 allocations | 10:44 |
| lubot_ | [telegram] <RikMills> // whilst without a reserve the internal exponential growth algorithm would do a better job | 10:44 |
| lubot_ | [telegram] <RikMills> list.reserve(list.size() + 2); | 10:44 |
| lubot_ | [telegram] <RikMills> for (int j = 0; j < 2; ++j) { | 10:44 |
| lubot_ | [telegram] <RikMills> list << m; | 10:44 |
| lubot_ | [telegram] <RikMills> } | 10:44 |
| lubot_ | [telegram] <RikMills> } | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> #### Supported containers | 10:44 |
| lubot_ | [telegram] <RikMills> `QVector`, `std::vector`, `QList`, `QSet` and `QVarLengthArray` | 10:44 |
| lubot_ | [telegram] <RikMills> | 10:44 |
| lubot_ | [telegram] <RikMills> #### Pitfalls | 10:44 |
| lubot_ | [telegram] <RikMills> clazy/docs/checks/README-reserve-candidates.md | 10:44 |
| lubot_ | [telegram] <RikMills> building qtbase with that commit reverted to test | 11:03 |
| lubot_ | [telegram] <tsimonq2> I can concur, that's about as far as I got (re @mitya57: I think clazy no longer generates the warning it needs to generate, maybe because it was relying on some QVector implementation detail that changed now. But that's just a hypothesis.) | 14:20 |
| lubot_ | [telegram] <RikMills> build time test still fails with that reverted in qtbase (re @RikMills: building qtbase with that commit reverted to test) | 14:26 |
| lubot_ | [telegram] <tsimonq2> FWIW swapping that test to a QList didn't work either | 14:33 |
| lubot_ | [telegram] <RikMills> as usual, not sure any other distros bother to run the tests 🙄 | 15:14 |
| lubot_ | [telegram] <tsimonq2> I figured it out | 19:43 |
| lubot_ | [telegram] <tsimonq2> https://code.qt.io/cgit/qt/qtbase.git/commit/src/corelib?h=5.15&id=826e076ef53a5809dee7d887b16ed961895049ea | 19:43 |
| lubot_ | [telegram] <tsimonq2> /src/checks/manuallevel/reserve-candidates.cpp:313 | 19:43 |
| lubot_ | [telegram] <RikMills> fix? | 19:58 |
| lubot_ | [telegram] <tsimonq2> confirming that the fix is to remove the test | 20:17 |
| lubot_ | [telegram] <mitya57> Removing the test doesn't sound right. Can you try replacing `foreach (int i2, d)` with `for (int i2 : d)`? | 20:39 |
| lubot_ | [telegram] <mitya57> Or maybe even better idea. Try this please: | 20:42 |
| lubot_ | [telegram] <mitya57> ``` | 20:42 |
| lubot_ | [telegram] <mitya57> --- a/src/checks/manuallevel/reserve-candidates.cpp | 20:42 |
| lubot_ | [telegram] <mitya57> +++ b/src/checks/manuallevel/reserve-candidates.cpp | 20:42 |
| lubot_ | [telegram] <mitya57> @@ -164,7 +164,7 @@ void ReserveCandidates::VisitStmt(clang::Stmt *stm) | 20:42 |
| lubot_ | [telegram] <mitya57> if (!body) | 20:42 |
| lubot_ | [telegram] <mitya57> return; | 20:42 |
| lubot_ | [telegram] <mitya57> | 20:42 |
| lubot_ | [telegram] <mitya57> - const bool isForeach = clazy::isInMacro(&m_astContext, clazy::getLocStart(stm), "Q_FOREACH"); | 20:42 |
| lubot_ | [telegram] <mitya57> + const bool isForeach = clazy::isInMacro(&m_astContext, clazy::getLocStart(stm), "Q_FOREACH_IMPL"); | 20:42 |
| lubot_ | [telegram] <mitya57> | 20:42 |
| lubot_ | [telegram] <mitya57> // If the body is another loop, we have nesting, ignore it now since the inner loops will be visited soon. | 20:42 |
| lubot_ | [telegram] <mitya57> if (isa<DoStmt>(body) || isa<WhileStmt>(body) || (!isForeach && isa<ForStmt>(body)))``` | 20:42 |
| lubot_ | [telegram] <tsimonq2> ack :) | 20:43 |
| lubot_ | [telegram] <tsimonq2> This works, but for loops are already tested for (re @mitya57: Removing the test doesn't sound right. Can you try replacing foreach (int i2, d) with for (int i2 : d)?) | 20:54 |
| lubot_ | [telegram] <tsimonq2> This does not work, unfortunately (re @mitya57: Or maybe even better idea. Try this please: | 20:54 |
| lubot_ | [telegram] <tsimonq2> --- a/src/checks/manuallevel/reserve-candidates.cpp | 20:54 |
| lubot_ | [telegram] <tsimonq2> +++ b/src/checks/manuallevel/reserve-candidates.cpp | 20:54 |
| lubot_ | [telegram] <tsimonq2> @@ -164,7 +164,7 @@ void ReserveCandidates::VisitStmt(clang::Stmt *stm) | 20:55 |
| lubot_ | [telegram] <tsimonq2> if (!body) | 20:55 |
| lubot_ | [telegram] <tsimonq2> return; | 20:55 |
| lubot_ | [telegram] <tsimonq2> | 20:55 |
| lubot_ | [telegram] <tsimonq2> - const bool isForeach = clazy::isInMacro(&m_astContext, clazy::getLocStart(stm), "Q_FOREACH"); | 20:55 |
| lubot_ | [telegram] <tsimonq2> + const bool isForeach = clazy::isInMacro(&m_astContext, clazy::getLocStart(stm), "Q_FOREACH_IMPL"); | 20:55 |
| lubot_ | [telegram] <tsimonq2> | 20:55 |
| lubot_ | [telegram] <tsimonq2> // If the body is another loop, we have nesting, ignore it now since the inner loops will be visited soon. | 20:55 |
| lubot_ | [telegram] <tsimonq2> if (isa<DoStmt>(body) || isa<WhileStmt>(body) || (!isForeach && isa<ForStmt>(body)))) | 20:55 |
| lubot_ | [telegram] <mitya57> Ah, I see | 20:57 |
| lubot_ | [telegram] <mitya57> ``` | 20:57 |
| lubot_ | [telegram] <mitya57> void rangeLoop() | 20:57 |
| lubot_ | [telegram] <mitya57> { | 20:57 |
| lubot_ | [telegram] <mitya57> QVector<int> v1, v2; | 20:57 |
| lubot_ | [telegram] <mitya57> for (auto i : v1) | 20:57 |
| lubot_ | [telegram] <mitya57> v2.push_back(i); | 20:57 |
| lubot_ | [telegram] <mitya57> }``` | 20:57 |
| lubot_ | [telegram] <mitya57> (re @tsimonq2: This works, but for loops are already tested for) | 20:57 |
| lubot_ | [telegram] <mitya57> OK, then I don't have better ideas, so maybe disabling is the only option. (re @tsimonq2: This does not work, unfortunately) | 20:58 |
| lubot_ | [telegram] <mitya57> tests/reserve-candidates/config.json has `"minimum_qt_version" : 50300` for this test, maybe you can add `"maximum_qt_version" : 51507`. | 20:59 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!