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.
> 
> Olivier Jean de Gaalon wrote:
>     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.
> 
> Sergey Kalinichev wrote:
>     > 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).
>     
>     That's exactly why I did it like this in the first place!
>     
>     > I just want to understand better how you're positioning IADI
>     
>     Sorry, I don't get it, what do you mean by "how you're positioning IADI"?

> That's exactly why I did it like this in the first place!

Yes, and I am agreeing with your implementation. Patch is good :).

My only question was about your description/explanation of the patch: I just see this as a cleanup of duplication, but it sounded like you wanted to split up IBSM into various other interfaces. I see that's not actually the case, and your point was just that the CustomBuildSystemManager now doesn't need to be a provider since that component was generalized. I guess that makes sense, though it's somewhat of a special case... every other buildsystem is going to also be a IDefinesAndIncludesManager::Provider.


- Olivier Jean de


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


On April 29, 2014, 11:21 a.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117836/
> -----------------------------------------------------------
> 
> (Updated April 29, 2014, 11:21 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.
> 
> 
> 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/20140429/9cf60099/attachment.html>