Re: [PATCH 04/34] kernelshark: Do not create a temporary container for looping over QMap

From: Benjamin ROBIN <dev@benjarobin.fr>
To: Yordan Karadzhov <y.karadz@gmail.com>
Cc: <linux-trace-devel@vger.kernel.org>, <dev@benjarobin.fr>
Subject: Re: [PATCH 04/34] kernelshark: Do not create a temporary container for looping over QMap
Date: Sun, 4 Feb 2024 19:59:08 +0100	[thread overview]
Message-ID: <gqbzlzqe3pss4opieve4c35xjgq7fgdgjh6iiq3zdm2ei5osnw@ac3adq75s3ez> (raw)
In-Reply-To: <bc2f74e7-6e1b-7f2e-40e0-b40c842a7bb4@gmail.com>

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]);