| Message ID | 20240114171723.14092-5-dev@benjarobin.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4f3bc9bb1fc0ebd295882d9d1331fb6de7867c0f |
| Headers | show |
| Series | Fix kernelshark issues introduced by the migration to Qt6 | expand |
On 1/14/24 19:16, Benjamin ROBIN wrote: > Use const_iterator instead. Fix container-anti-pattern Clazy warning > > Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr> > --- > src/KsAdvFilteringDialog.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/KsAdvFilteringDialog.cpp b/src/KsAdvFilteringDialog.cpp > index 4683c3d..247f912 100644 > --- a/src/KsAdvFilteringDialog.cpp > +++ b/src/KsAdvFilteringDialog.cpp > @@ -276,8 +276,8 @@ void KsAdvFilteringDialog::_makeFilterTable() > headers << "Delete" << "Stream" << "Event" << " Id" << "Filter"; > _table->init(headers, _filters.count()); > > - for(auto f : _filters.keys()) { > - QStringList thisFilter = _filters.value(f).split(":"); > + for (auto it = _filters.cbegin(), end = _filters.cend(); it != end; ++it) { Do we need to use iterator here? Perhaps you can do something like: for (const auto &[key, val] : _filters) { Thanks! Y. > + QStringList thisFilter = it.value().split(":"); > > i1 = new QTableWidgetItem(thisFilter[0]); > _table->setItem(count, 1, i1); > @@ -285,7 +285,7 @@ void KsAdvFilteringDialog::_makeFilterTable() > i1 = new QTableWidgetItem(thisFilter[1]); > _table->setItem(count, 2, i1); > > - i2 = new QTableWidgetItem(tr("%1").arg(f)); > + i2 = new QTableWidgetItem(tr("%1").arg(it.key())); > _table->setItem(count, 3, i2); > > i3 = new QTableWidgetItem(thisFilter[2]);
On Sun, Jan 21, 2024 at 07:16:01PM +0200, Yordan Karadzhov wrote: > > > On 1/14/24 19:16, Benjamin ROBIN wrote: > > Use const_iterator instead. Fix container-anti-pattern Clazy warning > > > > Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr> > > --- > > src/KsAdvFilteringDialog.cpp | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/src/KsAdvFilteringDialog.cpp b/src/KsAdvFilteringDialog.cpp > > index 4683c3d..247f912 100644 > > --- a/src/KsAdvFilteringDialog.cpp > > +++ b/src/KsAdvFilteringDialog.cpp > > @@ -276,8 +276,8 @@ void KsAdvFilteringDialog::_makeFilterTable() > > headers << "Delete" << "Stream" << "Event" << " Id" << "Filter"; > > _table->init(headers, _filters.count()); > > - for(auto f : _filters.keys()) { > > - QStringList thisFilter = _filters.value(f).split(":"); > > + for (auto it = _filters.cbegin(), end = _filters.cend(); it != end; ++it) { > > Do we need to use iterator here? > Perhaps you can do something like: > > for (const auto &[key, val] : _filters) { Unfortunately, you cannot do that, as far as I know (this not compile). C++ range-based for loop is using iterator behind the scene. See [1]. And there is still the problem of range-loop-detach (see patch 0005). Using const iterator is the most explicit and safe way to iterate over a container in Qt, so why not using it? [1] https://en.cppreference.com/w/cpp/language/range-for > Thanks! > Y. > > > + QStringList thisFilter = it.value().split(":"); > > i1 = new QTableWidgetItem(thisFilter[0]); > > _table->setItem(count, 1, i1); > > @@ -285,7 +285,7 @@ void KsAdvFilteringDialog::_makeFilterTable() > > i1 = new QTableWidgetItem(thisFilter[1]); > > _table->setItem(count, 2, i1); > > - i2 = new QTableWidgetItem(tr("%1").arg(f)); > > + i2 = new QTableWidgetItem(tr("%1").arg(it.key())); > > _table->setItem(count, 3, i2); > > i3 = new QTableWidgetItem(thisFilter[2]);
On 1/28/24 22:30, Benjamin ROBIN wrote: > On Sun, Jan 21, 2024 at 07:16:01PM +0200, Yordan Karadzhov wrote: >> >> >> On 1/14/24 19:16, Benjamin ROBIN wrote: >>> Use const_iterator instead. Fix container-anti-pattern Clazy warning >>> >>> Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr> >>> --- >>> src/KsAdvFilteringDialog.cpp | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/KsAdvFilteringDialog.cpp b/src/KsAdvFilteringDialog.cpp >>> index 4683c3d..247f912 100644 >>> --- a/src/KsAdvFilteringDialog.cpp >>> +++ b/src/KsAdvFilteringDialog.cpp >>> @@ -276,8 +276,8 @@ void KsAdvFilteringDialog::_makeFilterTable() >>> headers << "Delete" << "Stream" << "Event" << " Id" << "Filter"; >>> _table->init(headers, _filters.count()); >>> - for(auto f : _filters.keys()) { >>> - QStringList thisFilter = _filters.value(f).split(":"); >>> + for (auto it = _filters.cbegin(), end = _filters.cend(); it != end; ++it) { >> >> Do we need to use iterator here? >> Perhaps you can do something like: >> >> for (const auto &[key, val] : _filters) { > > Unfortunately, you cannot do that, as far as I know (this not compile). > C++ range-based for loop is using iterator behind the scene. See [1]. > And there is still the problem of range-loop-detach (see patch 0005). > Using const iterator is the most explicit and safe way to iterate over a > container in Qt, so why not using it? I see your point. I am applying this patch together with all v2 patches you sent. Are you considering sending new version of the "[31/34] Fix comparison of integers of different signs warnings"? Thanks, Y. > > [1] https://en.cppreference.com/w/cpp/language/range-for > >> Thanks! >> Y. >> >>> + QStringList thisFilter = it.value().split(":"); >>> i1 = new QTableWidgetItem(thisFilter[0]); >>> _table->setItem(count, 1, i1); >>> @@ -285,7 +285,7 @@ void KsAdvFilteringDialog::_makeFilterTable() >>> i1 = new QTableWidgetItem(thisFilter[1]); >>> _table->setItem(count, 2, i1); >>> - i2 = new QTableWidgetItem(tr("%1").arg(f)); >>> + i2 = new QTableWidgetItem(tr("%1").arg(it.key())); >>> _table->setItem(count, 3, i2); >>> i3 = new QTableWidgetItem(thisFilter[2]);
On Sun, Feb 04, 2024 at 08:34:39PM +0200, Yordan Karadzhov wrote: > > > On 1/28/24 22:30, Benjamin ROBIN wrote: > > On Sun, Jan 21, 2024 at 07:16:01PM +0200, Yordan Karadzhov wrote: > > > > > > > > > On 1/14/24 19:16, Benjamin ROBIN wrote: > > > > Use const_iterator instead. Fix container-anti-pattern Clazy warning > > > > > > > > Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr> > > > > --- > > > > src/KsAdvFilteringDialog.cpp | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/KsAdvFilteringDialog.cpp b/src/KsAdvFilteringDialog.cpp > > > > index 4683c3d..247f912 100644 > > > > --- a/src/KsAdvFilteringDialog.cpp > > > > +++ b/src/KsAdvFilteringDialog.cpp > > > > @@ -276,8 +276,8 @@ void KsAdvFilteringDialog::_makeFilterTable() > > > > headers << "Delete" << "Stream" << "Event" << " Id" << "Filter"; > > > > _table->init(headers, _filters.count()); > > > > - for(auto f : _filters.keys()) { > > > > - QStringList thisFilter = _filters.value(f).split(":"); > > > > + for (auto it = _filters.cbegin(), end = _filters.cend(); it != end; ++it) { > > > > > > Do we need to use iterator here? > > > Perhaps you can do something like: > > > > > > for (const auto &[key, val] : _filters) { > > > > Unfortunately, you cannot do that, as far as I know (this not compile). > > C++ range-based for loop is using iterator behind the scene. See [1]. > > And there is still the problem of range-loop-detach (see patch 0005). > > Using const iterator is the most explicit and safe way to iterate over a > > container in Qt, so why not using it? > > I see your point. I am applying this patch together with all v2 patches you > sent. > > Are you considering sending new version of the "[31/34] Fix comparison of > integers of different signs warnings"? I did not have time last weekend, nor this weekend to take a deep loop into it. Fixing types is not that easy: - I need a better understanding of the code base, otherwise I will break stuff. - I don't know if there are other projects that rely on these lib headers files. Is it possible to refactor every part of the project without breaking something else? - My patch tried to fix at various places warnings using mainly cast, but there are so many other places where the types are wrong (or at least not the optimal one). - This task is not that small. It requires a bit more time. So, in summary, this patch is currently on hold. I may work on it later this year... Sorry about that. Thanks, Benjamin > > > > [1] https://en.cppreference.com/w/cpp/language/range-for > > > Thanks! > > > Y. > > > > > > > + QStringList thisFilter = it.value().split(":"); > > > > i1 = new QTableWidgetItem(thisFilter[0]); > > > > _table->setItem(count, 1, i1); > > > > @@ -285,7 +285,7 @@ void KsAdvFilteringDialog::_makeFilterTable() > > > > i1 = new QTableWidgetItem(thisFilter[1]); > > > > _table->setItem(count, 2, i1); > > > > - i2 = new QTableWidgetItem(tr("%1").arg(f)); > > > > + i2 = new QTableWidgetItem(tr("%1").arg(it.key())); > > > > _table->setItem(count, 3, i2); > > > > i3 = new QTableWidgetItem(thisFilter[2]);
diff --git a/src/KsAdvFilteringDialog.cpp b/src/KsAdvFilteringDialog.cpp index 4683c3d..247f912 100644 --- a/src/KsAdvFilteringDialog.cpp +++ b/src/KsAdvFilteringDialog.cpp @@ -276,8 +276,8 @@ void KsAdvFilteringDialog::_makeFilterTable() headers << "Delete" << "Stream" << "Event" << " Id" << "Filter"; _table->init(headers, _filters.count()); - for(auto f : _filters.keys()) { - QStringList thisFilter = _filters.value(f).split(":"); + for (auto it = _filters.cbegin(), end = _filters.cend(); it != end; ++it) { + QStringList thisFilter = it.value().split(":"); i1 = new QTableWidgetItem(thisFilter[0]); _table->setItem(count, 1, i1); @@ -285,7 +285,7 @@ void KsAdvFilteringDialog::_makeFilterTable() i1 = new QTableWidgetItem(thisFilter[1]); _table->setItem(count, 2, i1); - i2 = new QTableWidgetItem(tr("%1").arg(f)); + i2 = new QTableWidgetItem(tr("%1").arg(it.key())); _table->setItem(count, 3, i2); i3 = new QTableWidgetItem(thisFilter[2]);
Use const_iterator instead. Fix container-anti-pattern Clazy warning Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr> --- src/KsAdvFilteringDialog.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)