Review Request: DrKonqi parses bactraces to find duplicates

[Matthias Fuchs]

> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/parsebugbacktraces.h, lines 66-151
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39319#file39319line66>
> >
> >     These functions aren't really templates. They kind of rely on Iter being a QList<BacktraceLine>::const_iterator. I am ok with making them templates just for avoiding typing the whole QList<BactraceLine>::const_iterator, but actually the same effect could be achieved with a typedef. In any case, they do not need to be in the header. It makes compilation slower and doesn't achieve anything. Just move them in the .cpp, even if you leave them as templates.

The reason I made them templates was that I had problems compiling the code without. Getting errors like

CMakeFiles/drkonqi.dir/duplicatefinderjob.o: In function `rating(QList<BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator, Q List<BacktraceLine>::const_iterator)':
/home/mat-not/kde/src/kde-runtime/drkonqi/parsebugbacktraces.h:76: multiple definition of `rating(QList<BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator, QList< BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator)'
CMakeFiles/drkonqi.dir/parsebugbacktraces.o:/home/mat-not/kde/src/kde-runtime/drkonqi/parsebugbacktraces.h:76: first defined here
collect2: ld returned 1 exit status
make[2]: *** [drkonqi/drkonqi] Error 1
make[1]: *** [drkonqi/CMakeFiles/drkonqi.dir/all] Error 2

Unfortunately I haven't sorted these out yet and just used templates instead.

In any case if I leave them templates they have to stay in the header, otherwise it would not compile and if it would it would not be standard conformant.


> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/parsebugbacktraces.h, lines 142-151
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39319#file39319line142>
> >
> >     This is quite clever and I like it, but it adds a dependency on boost, which is not an existing dependency of kde-runtime and you haven't added the right checks in CMakeLists.txt either. I would just replace them with for loops if I were you.

Ah, I thought this was used somewhere else.
I'll change that.


> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/parsebugbacktraces.cpp, line 28
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39320#file39320line28>
> >
> >     Yeah, realistically 99% of backtraces on bugzilla are in gdb format, but there can also be kdbgwin ones... But I think for the moment we can live with just gdb.

Indeed, I simply had no time to also implement the kdbgwin ones. :/


> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/reportassistantpages_bugzilla_duplicates.cpp, line 396
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39324#file39324line396>
> >
> >     unused variable?

No it is used below. The only reason I added that was to safe some typing. ;)
If you want I'll remove it in fact.


> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/reportassistantpages_bugzilla_duplicates.cpp, lines 404-405
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39324#file39324line404>
> >
> >     I take it that you want to color the whole line, right? It would be perhaps a better idea to use item->columnCount() instead of hardcoding.

Yes that is what was my intention.
You mean adding a loop over all columns and to do it there? --> I'll do that.


> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/reportassistantpages_bugzilla_duplicates.cpp, lines 411-415
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39324#file39324line411>
> >
> >     1) The KUIT thing should be "@info:label", iirc.
> >     2) I don't really understand the logic of it. If a report is open or it is RESOLVED/NEEDSINFO, then it can't possibly be marked as a duplicate. What am I missing?

ad 1) I used "@label" since that was already used in the code there. There is also a cheatsheet around, not sure if it is correct though: http://people.ubuntu.com/~apachelogger/misc/i18nccheatsheet.png

ad 2) The idea is that there exists already a bug which is either open or Resolved/Needsinfo, in that case the user should be asked to provide information there if they can. The status of the report itself won't be changed, this line should just lead to enhancing the existing report instead of creating a new one, which would in fact be a duplicate.


> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/reportinterface.h, lines 75-76
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39325#file39325line75>
> >
> >     Interesting, looks like this is the only place where a bug report number is a uint. (I think uint is the correct type, but everywhere else it's an int...)

I used uint since ReportInterface::attachToBugNumber also used uint.


> On Oct. 19, 2011, 10:35 p.m., George Kiagiadakis wrote:
> > drkonqi/duplicatefinderjob.h, line 80
> > <http://git.reviewboard.kde.org/r/102921/diff/1/?file=39317#file39317line80>
> >
> >     A more intuitive name for this function would be result() I think ;)

Ok. :)


On Oct. 19, 2011, 10:35 p.m., Matthias Fuchs wrote:
> > (Unrelated comment: Since I still consider myself the maintainer of drkonqi, could you please include me explicitly in the "People" field of review requests related to drkonqi in the future? It makes it easier for me to spot them, since it triggers a filter in my inbox. Thanks.)

Ok, sure. :)


- Matthias


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102921/#review7501
-----------------------------------------------------------


On Oct. 19, 2011, 6:58 p.m., Matthias Fuchs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102921/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2011, 6:58 p.m.)
> 
> 
> Review request for KDE Runtime.
> 
> 
> Description
> -------
> 
> This patch allows DrKonqi to download the possible duplicate bug reports and parse their backtraces.
> If a report turns out as duplicate reporting a new bug is not possible anymore.
> Thus the number of duplicates reported via DrKonqi should hopefully decline.
> 
> Note: The comparing backtraces part is not that good. Unfortunately I do not have much time to find a good algorithm. In the worst case we could make sure that only perfect duplicates are declined, that still should improve the current situation imo.
> 
> 
> Diffs
> -----
> 
>   drkonqi/CMakeLists.txt e0c2f6f 
>   drkonqi/bugzillalib.h 1e970a0 
>   drkonqi/bugzillalib.cpp aa558a9 
>   drkonqi/duplicatefinderjob.h PRE-CREATION 
>   drkonqi/duplicatefinderjob.cpp PRE-CREATION 
>   drkonqi/parsebugbacktraces.h PRE-CREATION 
>   drkonqi/parsebugbacktraces.cpp PRE-CREATION 
>   drkonqi/reportassistantdialog.cpp 43731b0 
>   drkonqi/reportassistantpages_base.cpp 4a25c45 
>   drkonqi/reportassistantpages_bugzilla_duplicates.h b0b4462 
>   drkonqi/reportassistantpages_bugzilla_duplicates.cpp 30e861d 
>   drkonqi/reportinterface.h c7f645b 
>   drkonqi/reportinterface.cpp 90e00b3 
>   drkonqi/ui/assistantpage_bugzilla_duplicates.ui 1ec034a 
> 
> Diff: http://git.reviewboard.kde.org/r/102921/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matthias Fuchs
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20111022/4c8c396e/attachment.htm>