Review Request: DrKonqi parses bactraces to find duplicates

Sun Oct 23 00:24:20 BST 2011

> 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.
> 
> Matthias Fuchs wrote:
>     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.

This error happens exactly because the function is in the header and not in the .cpp. If you make it non-template and put it in the .cpp, it should work just fine.

> 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.

This is not true. Templates don't need to be in the header to compile or be standards-compliant. We put their code in headers if we want them to be used from many places in the code, but if they are to be used only inside a certain source file, they can stay in that source file. It's like static functions in this case, they stay only in the source file and can't be used anywhere else, but they work just fine. Besides, C++ does not distinguish between headers and source files. The preprocessor merges all the headers in the source file and compiles them all as one big source file. It has no idea where a certain piece of code came from (well, it actually does, but only for debugging purposes)

> 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?
> 
> Matthias Fuchs wrote:
>     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.

Um, right, you are using it, but apparently not everywhere. In the if() clause you use m_result.status for example. Make everything use status instead.

> 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?
> 
> Matthias Fuchs wrote:
>     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.

1) ok, it's @label, my bad.
2) ok, I get it now. I was a bit confused with m_result.duplicate and m_result.parentDuplicate and the difference between them and how these are related with m_result.status, but now that I read it again it I understand it.

- George

-----------------------------------------------------------
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/4434d024/attachment.htm>