Review Request 117836: Now IDM (Includes/Defines manager) provides all types of includes/defines, not just user-specified. (kdevelop)

[Olivier Jean de Gaalon]

> On April 28, 2014, 6:53 p.m., Olivier Jean de Gaalon wrote:
> > I'd also like some clarification on the reasoning behind the patch, as you mention:
> > "Currently to retrieve i/d we use IBuildSystemManager, that's imo very strange, as I don't see any reason why e.g. builder() method and includes() should be in one interface? Language plugins obviously don't use the former..."
> > 
> > Language plugins aren't the only clients for IBuildSystemManager, builder() and includes() are in the same interface because they commonly share the same underlying data (provided by CMake, QMake, or any other buildsystem). That's IMO the best possible reason to decide what belongs in an interface. As to the custom buildsystem, I think it would make perfect sense to be able to manually define build targets as well as includes and defines, even if it doesn't right now.

To clarify my take on this a bit, I certainly see the interface duplication between IADI and IBSM and agree that it should be accessible from one place. It also makes sense to me to allow adding custom includes and defines on top of any IBSM without each IBSM having to custom-implement that functionality (and multiple providers is a fine generalization of that requirement). I just want to understand better how you're positioning IADI.


- Olivier Jean de


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117836/#review56789
-----------------------------------------------------------


On April 28, 2014, 11:04 a.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117836/
> -----------------------------------------------------------
> 
> (Updated April 28, 2014, 11:04 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
>   Currently to retrieve i/d we use IBuildSystemManager, that's imo very strange, as I don't see any reason why e.g. builder() method and includes() should be in one interface? Language plugins obviously don't use the former... 
>   Also not all project manager use i/d functionality (e.g. Custom build system doesn't).
>   Finally, with that path applied it becomes simpler to retrieve i/d as now all information is accessible from one place.
> 
>   There is one issue though: In CMakeManager::projectData() there is the assert with QThread::currentThread() == project->thread(), that I commented out. Currently it works without assertion, as processGeneratorExpression directly accesses m_projectsData, but with this path it becomes impossible to do that (only through friend class or something...).
>   So, is it ok to comment it out?
> 
> 
> Diffs
> -----
> 
>   languages/cpp/includepathcomputer.cpp 74fcc16 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.h bd560eb 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp d1723d3 
>   languages/plugins/custom-definesandincludes/kdevdefinesandincludesmanager.desktop.cmake 65b933f 
>   projectmanagers/cmake/cmakemanager.h 19fc0c1 
>   projectmanagers/cmake/cmakemanager.cpp 99e9e21 
>   projectmanagers/cmake/tests/cmakemanagertest.cpp 4e83175 
>   projectmanagers/custom-buildsystem/custombuildsystemplugin.h 0798602 
>   projectmanagers/custom-buildsystem/custombuildsystemplugin.cpp db194a4 
>   projectmanagers/custommake/custommakemanager.h 6946e62 
>   projectmanagers/custommake/custommakemanager.cpp 346c8cd 
> 
> Diff: https://git.reviewboard.kde.org/r/117836/diff/
> 
> 
> Testing
> -------
> 
> Everything language/project related still passes (except cmakeprojectvisitortest, but it doesn't work for me either way.)
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140428/25c38461/attachment.html>