Review Request 122905: Update kdev-valgrind to KF5 compatibility
Aleix Pol Gonzalez
aleixpol at kde.org
Sat Mar 14 03:58:50 UTC 2015
> On March 13, 2015, 12:54 a.m., Aleix Pol Gonzalez wrote:
> > models/memcheckitemsimpl.cpp, line 208
> > <https://git.reviewboard.kde.org/r/122905/diff/2/?file=354710#file354710line208>
> >
> > That's still not correct. Instead do the following:
> >
> > QUrl base = QUrl::fromLocalFile(QDir::cleanPath(dir+'/'));
> > QUrl url = base.resolved(QUrl(file));
> >
> > Also you could only do the resolution if QUrl(file).isRelative()
>
> Laszlo Kis-Adam wrote:
> It is always relative as the code provides just the filename in the file variable, so it's pointless to check if it's relative.
> Also the code works, valgrind provides the data in the format of directory and filename. The directory is always without trailing slash, and the filename is always just a filename.
>
> Here's an example of what memcheck provides, that is then parsed:
> <dir>/home/dfighter/projects/kdevtest/my/super/deep/example/directory</dir>
> <file>tests.cpp</file>
>
> As for the other valgrind tools, if they don't provide the right directory ( massif doesn't those items always have the directory set to the base directory ) the resolution will NOT work either way. So nothing lost, nothing gained.
>
> So what is incorrect exactly?
> Are you just being pedantic, or is there an actual use case where this is incorrect? If there is, ofc I will change it :)
> Thanks!
>
> Aleix Pol Gonzalez wrote:
> Well, the difference between being thorough and pedantic is can be thin. If you don't want to fix it, then don't.
>
> Laszlo Kis-Adam wrote:
> The question isn't whether I want to fix it or not, the question is whether there really is an issue. I don't think there is. I think this code works just fine.
> So let's bottomline this: will you merge it like this, or not? If you are only willing to merge it if I change it the way you suggested, then ofc I will change it.
Well, if I asked you to change it, was because I would have preferred to have optimal code that doesn't switch from and to QUrl for no reason. You could have saved your last message and done so, instead of continuing this.
I'll just commit this because this got into very useless bike-shedding for an otherwise good patch.
Let's leave this behind and keep an open mind.
- Aleix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122905/#review77402
-----------------------------------------------------------
On March 13, 2015, 12:02 a.m., Laszlo Kis-Adam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122905/
> -----------------------------------------------------------
>
> (Updated March 13, 2015, 12:02 a.m.)
>
>
> Review request for KDevelop.
>
>
> Repository: kdev-valgrind
>
>
> Description
> -------
>
> Updated kdev-valgrind to be compatible with KF5
>
>
> Diffs
> -----
>
> CMakeLists.txt 31aaf52
> cmake/FindKDevPlatform.cmake 1a771c5
> config.h a7ec923
> config.cpp 9924dca
> config/cachegrindconfigpage.h 070fc06
> config/cachegrindconfigpage.cpp a487cc5
> config/callgrindconfigpage.h eefbb18
> config/callgrindconfigpage.cpp 2c97477
> config/genericconfigpage.h e6c9108
> config/genericconfigpage.cpp d0147f9
> config/helgrindconfigpage.h 497e328
> config/helgrindconfigpage.cpp 3fd97bd
> config/massifconfigpage.h 64f0f11
> config/massifconfigpage.cpp 49660af
> config/memcheckconfigpage.h f1d5925
> config/memcheckconfigpage.cpp 3a661db
> config/ui/cachegrindconfig.ui 170731e
> config/ui/callgrindconfig.ui fbb003d
> config/ui/genericconfig.ui a09f301
> config/ui/helgrindconfig.ui 6cead3c
> config/ui/massifconfig.ui 6d4a8de
> config/ui/memcheckconfig.ui 11a163e
> config/valgrindcachegrindconfigpage.h 73f65de
> config/valgrindcachegrindconfigpage.cpp 22b349b
> config/valgrindcallgrindconfigpage.h fbdc90d
> config/valgrindcallgrindconfigpage.cpp 5820af3
> config/valgrindgenericconfigpage.h cdaf0a8
> config/valgrindgenericconfigpage.cpp e05ef52
> config/valgrindhelgrindconfigpage.h c7487c2
> config/valgrindhelgrindconfigpage.cpp b6e877d
> config/valgrindmassifconfigpage.h 62e3e79
> config/valgrindmassifconfigpage.cpp 9165f6e
> config/valgrindmemcheckconfigpage.h 431040a
> config/valgrindmemcheckconfigpage.cpp 7c4f92f
> debug.h PRE-CREATION
> debug.cpp PRE-CREATION
> job.h ec4b1b0
> job.cpp cf832c4
> jobs/cachegrindjob.cpp 0028a62
> jobs/callgrindjob.cpp 627e4f7
> jobs/massifjob.cpp 9fe2fde
> jobs/memcheckjob.cpp 6912760
> kdevvalgrind.desktop fb76bb6
> kdevvalgrind.desktop.cmake PRE-CREATION
> launcher.h 539e34f
> launcher.cpp d9b52af
> marks.cpp ff2c890
> models/cachegrinditem.h 311d228
> models/cachegrinditem.cpp 1b50b63
> models/cachegrindmodel.cpp 70cad22
> models/callgrinditem.h 11c4a33
> models/callgrinditem.cpp 845104b
> models/callgrindmodel.cpp 5dc971c
> models/massifitem.h 6460fba
> models/massifitem.cpp 33cd92d
> models/massifmodel.cpp 4d5f66a
> models/memcheckitemsimpl.h 111e4a6
> models/memcheckitemsimpl.cpp fdee8d2
> models/memcheckmodel.cpp 2b96fbe
> parsers/cachegrindparser.h 312f09a
> parsers/cachegrindparser.cpp f82fd8a
> parsers/callgrindparser.h e7f522d
> parsers/callgrindparser.cpp 5ffe4c9
> parsers/massifparser.h 1a97f3b
> parsers/memcheckparser.h 2e3d5ae
> parsers/memcheckparser.cpp cf438ea
> plugin.h e0410e4
> plugin.cpp 3cd2448
> statjob.h PRE-CREATION
> statjob.cpp PRE-CREATION
> views/cachegrindview.cpp 591f217
> views/callgrindview.cpp 6aa551d
> views/massifview.cpp 7bb275f
> views/memcheckview.cpp 95961fb
> widget.cpp 18eacf7
>
> Diff: https://git.reviewboard.kde.org/r/122905/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Laszlo Kis-Adam
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150314/1fa12d12/attachment.html>
More information about the KDevelop-devel
mailing list