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

[Olivier Jean de Gaalon]

> On May 2, 2014, 1:19 p.m., Milian Wolff wrote:
> > project/interfaces/ibuildsystemmanager.h, line 66
> > <https://git.reviewboard.kde.org/r/117835/diff/2/?file=270613#file270613line66>
> >
> >     because it is an interface
> 
> Sergey Kalinichev wrote:
>     I don't get it. I've just went through KDevelop's code base and it seems like pure interfaces are very rare, most provide default implementation for some functions, I see no reason why IBSM can't do it that way too, especially when we already have 2 plugins that don't use that functionality.

This is getting into serious bikeshedding territory, as you're right that it's not a big deal. Still, I'm with Milian on the color we're painting with, maybe for different reasons.

CustomMakeBuildsystem doesn't provide include paths because it's deficient, CustomBuildSystem is the exception here. In fact, I could see an argument for KDevelop to replace the CustomBuildSystem with parallel systems like you've created for the includes and defines. This would eventually allow one to override or enhance any aspect of the buildsystem plugin in use, or not use a buildsystem plugin at all.

It's a matter of perspective. Mine is that your changes provide useful customization features but they don't change the fact that it's the IBuildSystemManager's job to provide includes and defines. I wouldn't want to mainstream any buildsystem plugin that didn't implement these functions and depended on the user providing them. The correct way to encode that is with pure virtual.

Given that, ship it (from me)!


- Olivier Jean de


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


On May 1, 2014, 11:20 a.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117835/
> -----------------------------------------------------------
> 
> (Updated May 1, 2014, 11:20 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> The changes include extending IDM interface and removing i/d notion from IBuildSystemManager interface.
> 
> 
> Diffs
> -----
> 
>   language/interfaces/idefinesandincludesmanager.h ab42444 
>   project/interfaces/ibuildsystemmanager.h 241b696 
>   project/interfaces/ibuildsystemmanager.cpp 74af0e7 
> 
> Diff: https://git.reviewboard.kde.org/r/117835/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140506/7218feb7/attachment-0001.html>